Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: REST API endpoints for Orders, Taxes, Shipping and Payment Management #2412

Open
wants to merge 6 commits into
base: update/vendor-dashboard-structure
Choose a base branch
from

Conversation

mralaminahamed
Copy link
Member

@mralaminahamed mralaminahamed commented Oct 24, 2024

All Submissions:

  • My code follow the WordPress' coding standards
  • My code satisfies feature requirements
  • My code is tested
  • My code passes the PHPCS tests
  • My code has proper inline documentation
  • I've included related pull request(s) (optional)
  • I've included developer documentation
  • I've added proper labels to this pull request

Changes proposed in this Pull Request:

This PR introduces new REST API endpoints to enhance the Dokan plugin's capabilities:

  1. Order Management APIs (v3):

    • Order CRUD operations with vendor authorization
    • Order notes management
    • Order refunds handling
    • Order actions (email notifications, download permissions)
  2. Tax Management APIs (v1):

    • Tax rates and classes endpoints
    • Vendor-specific tax settings
  3. Shipping Management APIs (v1):

    • Shipping methods endpoints
    • Shipping status tracking
    • Country/state data for shipping zones
  4. Payment Gateway APIs (v1):

    • Available payment methods listing
    • Gateway-specific settings

Each endpoint implements proper permission checks and follows WordPress REST API standards.

Related Pull Request(s)

N/A

Closes

N/A

How to test the changes in this Pull Request:

  1. Install and activate the plugin
  2. Use a REST API client (e.g., Postman)
  3. Test endpoints with vendor credentials:

Orders:

GET /wp-json/dokan/v3/orders
POST /wp-json/dokan/v3/orders
GET /wp-json/dokan/v3/orders/{id}

Taxes:

GET /wp-json/dokan/v1/taxes
GET /wp-json/dokan/v1/taxes/classes

Shipping:

GET /wp-json/dokan/v1/shipping_methods
GET /wp-json/dokan/v1/data/countries

Payment:

GET /wp-json/dokan/v1/payment_gateways

Changelog entry

Title: Enhanced REST API endpoints for vendor operations

Added comprehensive REST API endpoints for vendors to manage orders, taxes, shipping, and payments through standardized interfaces. New endpoints include order management with refund capabilities, tax rate configuration, shipping method setup, and payment gateway integration.

Before Changes

Vendors had limited API access to manage core functionalities like orders, taxes, shipping, and payments.

After Changes

Vendors can now programmatically:

  • Manage orders with full CRUD operations
  • Configure tax rates and classes
  • Set up shipping methods and track shipments
  • Access payment gateway settings

All endpoints implement proper authorization and follow WordPress REST API best practices.

PR Self Review Checklist:

  • Code follows style guidelines
  • Clear naming conventions used
  • Code keeps it simple (KISS)
  • No code duplication (DRY)
  • Code is readable and maintainable
  • Performance considerations addressed
  • Complex logic is documented
  • No grammar errors

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced multiple REST API controllers for managing various aspects of the Dokan plugin, including countries, orders, order actions, order notes, refunds, payment gateways, shipping methods, and taxes.
    • Enhanced permission checks to ensure that only authorized vendors can access and manage their respective resources.
  • Tests

    • Added comprehensive test suites for the newly introduced controllers to validate functionality, authorization, and error handling across various scenarios.

@mralaminahamed mralaminahamed self-assigned this Oct 24, 2024
Copy link
Contributor

coderabbitai bot commented Oct 24, 2024

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This pull request introduces several new PHP classes and test suites to enhance the REST API functionality within the Dokan plugin. Key additions include controllers for managing country data, orders, order actions, order notes, order refunds, payment gateways, shipping methods, and tax classes. Each controller implements specific permission checks to ensure that only authorized users can access or modify resources. Additionally, comprehensive test classes are added to validate the functionality of these controllers, ensuring robust error handling and compliance with expected behaviors.

Changes

File Path Change Summary
includes/REST/DataCountryController.php Added DataCountryController class with permission checks for accessing country data.
includes/REST/Manager.php Updated get_rest_api_class_map method to include new controller mappings.
includes/REST/OrderActionControllerV3.php Added OrderActionControllerV3 class with methods for managing order actions and permissions.
includes/REST/OrderControllerV3.php Introduced OrderControllerV3 class for order management with comprehensive permission checks.
includes/REST/OrderNoteControllerV3.php Added OrderNoteControllerV3 class for managing order notes with authorization checks.
includes/REST/OrderRefundControllerV3.php Introduced OrderRefundControllerV3 class for handling order refunds with vendor permissions.
includes/REST/PaymentGatewayController.php Added PaymentGatewayController class with permission checks for payment gateway access.
includes/REST/ShippingMethodController.php Introduced ShippingMethodController class for managing shipping methods with permission checks.
includes/REST/TaxClassController.php Added TaxClassController class with permission checks for accessing tax classes.
includes/REST/TaxController.php Introduced TaxController class for managing tax-related operations with permission checks.
tests/php/src/REST/DataCountriesControllerTest.php Added test suite for DataCountryController with various test cases for API functionality.
tests/php/src/REST/OrdersControllerV3Test.php Introduced test suite for order management API with comprehensive coverage of functionalities.
tests/php/src/REST/PaymentGatewaysControllerTest.php Added test suite for payment gateways API with validation of route registration and data retrieval.
tests/php/src/REST/ShippingMethodControllerTest.php Introduced test suite for shipping methods API with validation of route registration and data retrieval.
tests/php/src/REST/TaxClassControllerTest.php Added test suite for tax classes API with validation of route registration and data retrieval.
tests/php/src/REST/TaxControllerTest.php Introduced test suite for tax-related API functionality with route verification and data retrieval.
tests/php/src/REST/OrderActionControllerV3Test.php Added test suite for order actions API with comprehensive validation of functionalities.
tests/php/src/REST/OrderNoteControllerV3Test.php Introduced test suite for order notes API with thorough coverage of functionalities.
tests/php/src/REST/OrderRefundControllerV3Test.php Added test suite for order refunds API with extensive validation of functionalities.

Possibly related PRs

Suggested labels

Ready to Merge, :+1: Dev Review Done, Upcoming Release, Changelog Updated

Suggested reviewers

  • mrabbani

🐇 In the land of code where rabbits play,
New controllers hop in, brightening the day!
With tests to ensure all functions align,
Our Dokan plugin shines, oh so divine!
Permissions checked, all errors caught,
A joyful release, just as we thought! 🐇


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@mralaminahamed mralaminahamed added Needs: Testing This requires further testing Needs: Dev Review It requires a developer review and approval labels Oct 24, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.

🛑 Comments failed to post (52)
tests/php/src/REST/TaxClassControllerTest.php (3)

19-25: 🛠️ Refactor suggestion

Enhance route registration test coverage.

The current test only verifies the GET method. Consider expanding the test to:

  1. Verify all supported HTTP methods
  2. Assert route parameters and arguments
  3. Validate permission callback registration

Here's a suggested enhancement:

 public function test_ensure_route_is_registered() {
     $routes = $this->server->get_routes( $this->namespace );
     $full_route = $this->get_route( 'taxes/classes' );

     $this->assertArrayHasKey( $full_route, $routes );
-    $this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] );
+    $route = $routes[$full_route][0];
+    
+    // Verify supported methods
+    $this->assertArrayHasKey( 'GET', $route['methods'] );
+    
+    // Verify permission callback exists
+    $this->assertArrayHasKey( 'permission_callback', $route );
+    $this->assertTrue( is_callable( $route['permission_callback'] ) );
+    
+    // Verify arguments schema
+    $this->assertArrayHasKey( 'args', $route );
 }

Committable suggestion was skipped due to low confidence.


30-38: 🛠️ Refactor suggestion

Expand test coverage for tax classes retrieval.

The current test only covers the basic success case. Consider adding:

  1. Test cases for different user roles (admin, customer, guest)
  2. Validation of response data structure and content
  3. Error cases (unauthorized access, invalid requests)
  4. Edge cases (empty tax classes, maximum limits)

Here's a suggested enhancement:

 public function test_get_tax_classes() {
     wp_set_current_user( $this->seller_id1 );
     $response = $this->get_request( 'taxes/classes' );
     
     $this->assertEquals( 200, $response->get_status() );
     $data = $response->get_data();
     $this->assertIsArray( $data );
+    
+    // Validate data structure
+    foreach ($data as $tax_class) {
+        $this->assertArrayHasKey('name', $tax_class);
+        $this->assertArrayHasKey('slug', $tax_class);
+    }
 }
+
+/**
+ * @testWith ["administrator", 200]
+ *           ["shop_manager", 200]
+ *           ["customer", 403]
+ *           [null, 401]
+ */
+public function test_get_tax_classes_permissions($role, $expected_status) {
+    if ($role) {
+        $user_id = $this->factory->user->create(['role' => $role]);
+        wp_set_current_user($user_id);
+    }
+    
+    $response = $this->get_request('taxes/classes');
+    $this->assertEquals($expected_status, $response->get_status());
+}
+
+public function test_get_tax_classes_empty() {
+    wp_set_current_user($this->seller_id1);
+    
+    // Remove all tax classes first
+    update_option('woocommerce_tax_classes', '');
+    
+    $response = $this->get_request('taxes/classes');
+    $this->assertEquals(200, $response->get_status());
+    $this->assertEmpty($response->get_data());
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    public function test_get_tax_classes() {
        wp_set_current_user( $this->seller_id1 );
        $response = $this->get_request( 'taxes/classes' );
        
        $this->assertEquals( 200, $response->get_status() );
        $data = $response->get_data();
        $this->assertIsArray( $data );
        
        // Validate data structure
        foreach ($data as $tax_class) {
            $this->assertArrayHasKey('name', $tax_class);
            $this->assertArrayHasKey('slug', $tax_class);
        }
    }

    /**
     * @testWith ["administrator", 200]
     *           ["shop_manager", 200]
     *           ["customer", 403]
     *           [null, 401]
     */
    public function test_get_tax_classes_permissions($role, $expected_status) {
        if ($role) {
            $user_id = $this->factory->user->create(['role' => $role]);
            wp_set_current_user($user_id);
        }
        
        $response = $this->get_request('taxes/classes');
        $this->assertEquals($expected_status, $response->get_status());
    }

    public function test_get_tax_classes_empty() {
        wp_set_current_user($this->seller_id1);
        
        // Remove all tax classes first
        update_option('woocommerce_tax_classes', '');
        
        $response = $this->get_request('taxes/classes');
        $this->assertEquals(200, $response->get_status());
        $this->assertEmpty($response->get_data());
    }

8-12: 💡 Codebase verification

Now I have enough context to provide the final response:

The @group annotation dokan-tax-classes-v3 appears to be incorrect

The TaxClassController implementation in includes/REST/TaxClassController.php clearly uses the dokan/v1 namespace, which matches the test file's namespace. While there are v3 endpoints in the codebase, they are specifically for order-related functionality (OrderController, OrderNoteController, etc.). The tax classes API is intentionally implemented as a v1 endpoint.

Recommendation:

  • Update the test group annotation from @group dokan-tax-classes-v3 to @group dokan-tax-classes-v1 to maintain consistency with the actual implementation.
🔗 Analysis chain

Version mismatch between @group annotation and namespace.

The @group annotation suggests v3 (dokan-tax-classes-v3), but the namespace is set to v1. This inconsistency could lead to confusion. Please verify the correct version and update either the annotation or the namespace accordingly.


Let me gather more context about the tax-related endpoints and their versioning by checking the actual controller implementation.


Let me check the actual implementation of the TaxClassController to understand the versioning strategy.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for version consistency across tax-related files
# Expected: All tax-related endpoints should use the same version

# Check for tax-related routes and their versions
rg -A 5 "namespace.*dokan/v[0-9].*tax" --type php
rg -A 5 "@group.*tax.*v[0-9]" --type php

Length of output: 499


Script:

#!/bin/bash
# Check for the actual TaxClass controller implementation and its versioning
rg -l "class.*TaxClass.*Controller" --type php

# Look for tax-related REST route registrations
ast-grep --pattern 'register_rest_route($_, "dokan/v$_/tax"'

Length of output: 194


Script:

#!/bin/bash
# Check the content of TaxClassController.php for namespace and route registration
cat includes/REST/TaxClassController.php

# Also check if there are any other v3 endpoints in the codebase for context
rg "dokan/v3" --type php

Length of output: 2437

tests/php/src/REST/TaxControllerTest.php (2)

19-25: ⚠️ Potential issue

Expand route registration test coverage.

The current test only verifies GET method registration. For a tax management API, we should also:

  1. Verify other expected HTTP methods (POST/PUT/DELETE)
  2. Validate route parameters and schema
  3. Verify that unauthorized HTTP methods are not registered

Here's a suggested enhancement:

 public function test_ensure_route_is_registered() {
     $routes = $this->server->get_routes( $this->namespace );
     $full_route = $this->get_route( 'taxes' );

     $this->assertArrayHasKey( $full_route, $routes );
-    $this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] );
+    $route_data = $routes[$full_route][0];
+    
+    // Verify allowed methods
+    $this->assertTrue( $route_data['methods']['GET'] );
+    $this->assertTrue( $route_data['methods']['POST'] );
+    $this->assertTrue( $route_data['methods']['PUT'] );
+    $this->assertTrue( $route_data['methods']['DELETE'] );
+    
+    // Verify schema
+    $this->assertArrayHasKey( 'schema', $route_data );
+    $this->assertEquals( 'dokan_tax_rate', $route_data['schema']['title'] );
+    
+    // Verify args for GET
+    $this->assertArrayHasKey( 'args', $route_data );
+    $this->assertArrayHasKey( 'context', $route_data['args'] );
 }

Committable suggestion was skipped due to low confidence.


30-39: ⚠️ Potential issue

Enhance tax retrieval test coverage.

The current test only performs basic validation. Consider adding:

  1. Test data setup with known tax rates
  2. Specific assertions about tax data structure
  3. Error cases (unauthorized users, invalid requests)
  4. Vendor-specific tax data validation

Here's a suggested enhancement:

 public function test_get_taxes() {
+    // Setup test data
+    $tax_rate = array(
+        'country'  => 'US',
+        'state'    => 'AL',
+        'rate'     => '7.5',
+        'name'     => 'State Tax',
+        'priority' => 1,
+        'compound' => false,
+    );
+    $tax_rate_id = \WC_Tax::_insert_tax_rate($tax_rate);
+
     wp_set_current_user( $this->seller_id1 );

     $response = $this->get_request( 'taxes' );

     $this->assertEquals( 200, $response->get_status() );
     $data = $response->get_data();
     $this->assertIsArray( $data );
-    // Add more assertions based on expected tax data
+    
+    // Verify tax data structure
+    $this->assertArrayHasKey( 'id', $data[0] );
+    $this->assertArrayHasKey( 'country', $data[0] );
+    $this->assertArrayHasKey( 'state', $data[0] );
+    $this->assertArrayHasKey( 'rate', $data[0] );
+    $this->assertArrayHasKey( 'name', $data[0] );
+    
+    // Verify tax data values
+    $this->assertEquals( $tax_rate['country'], $data[0]['country'] );
+    $this->assertEquals( $tax_rate['rate'], $data[0]['rate'] );
+    
+    // Clean up
+    \WC_Tax::_delete_tax_rate($tax_rate_id);
 }
+
+/**
+ * Test tax retrieval with unauthorized user
+ */
+public function test_get_taxes_unauthorized() {
+    wp_set_current_user( 0 );
+    $response = $this->get_request( 'taxes' );
+    $this->assertEquals( 401, $response->get_status() );
+}
+
+/**
+ * Test vendor-specific tax data
+ */
+public function test_get_vendor_specific_taxes() {
+    wp_set_current_user( $this->seller_id1 );
+    
+    // Setup vendor-specific tax settings
+    update_user_meta( $this->seller_id1, 'dokan_tax_rate', '8.5' );
+    
+    $response = $this->get_request( 'taxes' );
+    $data = $response->get_data();
+    
+    $this->assertEquals( '8.5', $data[0]['rate'] );
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    public function test_get_taxes() {
        // Setup test data
        $tax_rate = array(
            'country'  => 'US',
            'state'    => 'AL',
            'rate'     => '7.5',
            'name'     => 'State Tax',
            'priority' => 1,
            'compound' => false,
        );
        $tax_rate_id = \WC_Tax::_insert_tax_rate($tax_rate);

        wp_set_current_user( $this->seller_id1 );

        $response = $this->get_request( 'taxes' );

        $this->assertEquals( 200, $response->get_status() );
        $data = $response->get_data();
        $this->assertIsArray( $data );
        
        // Verify tax data structure
        $this->assertArrayHasKey( 'id', $data[0] );
        $this->assertArrayHasKey( 'country', $data[0] );
        $this->assertArrayHasKey( 'state', $data[0] );
        $this->assertArrayHasKey( 'rate', $data[0] );
        $this->assertArrayHasKey( 'name', $data[0] );
        
        // Verify tax data values
        $this->assertEquals( $tax_rate['country'], $data[0]['country'] );
        $this->assertEquals( $tax_rate['rate'], $data[0]['rate'] );
        
        // Clean up
        \WC_Tax::_delete_tax_rate($tax_rate_id);
    }

    /**
     * Test tax retrieval with unauthorized user
     */
    public function test_get_taxes_unauthorized() {
        wp_set_current_user( 0 );
        $response = $this->get_request( 'taxes' );
        $this->assertEquals( 401, $response->get_status() );
    }

    /**
     * Test vendor-specific tax data
     */
    public function test_get_vendor_specific_taxes() {
        wp_set_current_user( $this->seller_id1 );
        
        // Setup vendor-specific tax settings
        update_user_meta( $this->seller_id1, 'dokan_tax_rate', '8.5' );
        
        $response = $this->get_request( 'taxes' );
        $data = $response->get_data();
        
        $this->assertEquals( '8.5', $data[0]['rate'] );
    }
tests/php/src/REST/PaymentGatewaysControllerTest.php (3)

14-25: 🛠️ Refactor suggestion

Enhance route registration test coverage.

While the basic route registration is tested, consider adding:

  1. Validation of route parameters and schema
  2. Tests for other HTTP methods if supported (POST, PUT, DELETE)
  3. Verification of permission callbacks
public function test_ensure_route_is_registered() {
    $routes = $this->server->get_routes($this->namespace);
    $full_route = $this->get_route('payment_gateways');
    $route_data = $routes[$full_route][0];

    $this->assertArrayHasKey($full_route, $routes);
    $this->assertNestedContains(['methods' => ['GET' => true]], $routes[$full_route]);
    
    // Verify schema
    $this->assertArrayHasKey('schema', $route_data);
    
    // Verify permissions
    $this->assertArrayHasKey('permission_callback', $route_data);
}

27-39: 🛠️ Refactor suggestion

Enhance payment gateways list test coverage.

The current test only covers the happy path. Consider adding:

  1. Response structure validation
  2. Error cases (unauthorized access, no gateways configured)
  3. Pagination testing if supported
public function test_get_payment_gateways() {
    wp_set_current_user($this->seller_id1);
    $response = $this->get_request('payment_gateways');
    
    $this->assertEquals(200, $response->get_status());
    $data = $response->get_data();
    
    // Validate structure
    $this->assertArrayHasKey('id', $data[0]);
    $this->assertArrayHasKey('title', $data[0]);
    $this->assertArrayHasKey('enabled', $data[0]);
    
    // Test unauthorized access
    wp_set_current_user(0);
    $response = $this->get_request('payment_gateways');
    $this->assertEquals(401, $response->get_status());
}

41-54: 🛠️ Refactor suggestion

Improve single gateway test robustness.

The test uses hardcoded 'bacs' gateway ID and lacks error cases. Consider:

  1. Using test data constants or setup fixtures
  2. Testing invalid gateway IDs
  3. Validating complete response structure
private const TEST_GATEWAY = 'bacs';

public function test_get_payment_gateway() {
    wp_set_current_user($this->seller_id1);
    
    // Test valid gateway
    $response = $this->get_request('payment_gateways/' . self::TEST_GATEWAY);
    $this->assertEquals(200, $response->get_status());
    $data = $response->get_data();
    
    // Validate complete structure
    $this->assertEquals(self::TEST_GATEWAY, $data['id']);
    $this->assertArrayHasKey('title', $data);
    $this->assertArrayHasKey('description', $data);
    $this->assertArrayHasKey('enabled', $data);
    $this->assertArrayHasKey('settings', $data);
    
    // Test invalid gateway
    $response = $this->get_request('payment_gateways/invalid_gateway');
    $this->assertEquals(404, $response->get_status());
}
tests/php/src/REST/DataCountriesControllerTest.php (3)

45-54: 🛠️ Refactor suggestion

Expand country states test coverage.

The test should include:

  1. Verification of specific state data structure
  2. Invalid country code handling
  3. Error scenarios
 public function test_get_country_states() {
     wp_set_current_user( $this->seller_id1 );

     $response = $this->get_request( 'data/countries/US' );

     $this->assertEquals( 200, $response->get_status() );
     $data = $response->get_data();
     $this->assertArrayHasKey( 'states', $data );
     $this->assertNotEmpty( $data['states'] );
+    
+    // Verify specific states exist with correct format
+    $states = $data['states'];
+    $this->assertArrayHasKey('CA', $states);
+    $this->assertEquals('California', $states['CA']);
+    $this->assertArrayHasKey('NY', $states);
+    $this->assertEquals('New York', $states['NY']);
 }
+
+/**
+ * Test invalid country code for states endpoint.
+ */
+public function test_get_country_states_invalid_country() {
+    wp_set_current_user( $this->seller_id1 );
+    
+    $response = $this->get_request( 'data/countries/INVALID' );
+    $this->assertEquals( 404, $response->get_status() );
+}

Committable suggestion was skipped due to low confidence.


30-40: 🛠️ Refactor suggestion

Enhance countries endpoint test coverage.

The test verifies basic functionality but should be expanded to include:

  1. Specific country data structure validation
  2. Unauthorized access scenarios
  3. Pagination if supported
 public function test_get_countries() {
     wp_set_current_user( $this->seller_id1 );

     $response = $this->get_request( 'data/countries' );

     $this->assertEquals( 200, $response->get_status() );
     $data = $response->get_data();
     $this->assertIsArray( $data );
     $this->assertNotEmpty( $data );
-    // Add more assertions based on expected country data
+    
+    // Verify country data structure
+    $first_country = reset($data);
+    $this->assertArrayHasKey('code', $first_country);
+    $this->assertArrayHasKey('name', $first_country);
+    
+    // Known country verification
+    $us_exists = false;
+    foreach ($data as $country) {
+        if ($country['code'] === 'US') {
+            $us_exists = true;
+            $this->assertEquals('United States', $country['name']);
+            break;
+        }
+    }
+    $this->assertTrue($us_exists, 'US should exist in countries list');
 }

+/**
+ * Test unauthorized access to countries endpoint.
+ */
+public function test_get_countries_unauthorized() {
+    wp_set_current_user( 0 );
+    
+    $response = $this->get_request( 'data/countries' );
+    $this->assertEquals( 401, $response->get_status() );
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    public function test_get_countries() {
        wp_set_current_user( $this->seller_id1 );

        $response = $this->get_request( 'data/countries' );

        $this->assertEquals( 200, $response->get_status() );
        $data = $response->get_data();
        $this->assertIsArray( $data );
        $this->assertNotEmpty( $data );
        
        // Verify country data structure
        $first_country = reset($data);
        $this->assertArrayHasKey('code', $first_country);
        $this->assertArrayHasKey('name', $first_country);
        
        // Known country verification
        $us_exists = false;
        foreach ($data as $country) {
            if ($country['code'] === 'US') {
                $us_exists = true;
                $this->assertEquals('United States', $country['name']);
                break;
            }
        }
        $this->assertTrue($us_exists, 'US should exist in countries list');
    }

    /**
     * Test unauthorized access to countries endpoint.
     */
    public function test_get_countries_unauthorized() {
        wp_set_current_user( 0 );
        
        $response = $this->get_request( 'data/countries' );
        $this->assertEquals( 401, $response->get_status() );
    }

19-25: 🛠️ Refactor suggestion

Consider expanding route registration test coverage.

The current test verifies basic route registration and GET method support. Consider adding:

  1. Negative tests for invalid routes
  2. Verification of route arguments and callbacks
  3. Tests for other HTTP methods to ensure they're not allowed
 public function test_ensure_route_is_registered() {
     $routes = $this->server->get_routes( $this->namespace );
     $full_route = $this->get_route( 'data/countries' );

     $this->assertArrayHasKey( $full_route, $routes );
     $this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] );
+    
+    // Verify route arguments
+    $this->assertArrayHasKey( 'args', $routes[$full_route][0] );
+    
+    // Verify other methods are not allowed
+    $this->assertArrayNotHasKey( 'POST', $routes[$full_route][0]['methods'] );
+    $this->assertArrayNotHasKey( 'PUT', $routes[$full_route][0]['methods'] );
+    $this->assertArrayNotHasKey( 'DELETE', $routes[$full_route][0]['methods'] );
+    
+    // Test invalid route
+    $invalid_route = $this->get_route( 'data/invalid' );
+    $this->assertArrayNotHasKey( $invalid_route, $routes );
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    public function test_ensure_route_is_registered() {
        $routes = $this->server->get_routes( $this->namespace );
        $full_route = $this->get_route( 'data/countries' );

        $this->assertArrayHasKey( $full_route, $routes );
        $this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] );
        
        // Verify route arguments
        $this->assertArrayHasKey( 'args', $routes[$full_route][0] );
        
        // Verify other methods are not allowed
        $this->assertArrayNotHasKey( 'POST', $routes[$full_route][0]['methods'] );
        $this->assertArrayNotHasKey( 'PUT', $routes[$full_route][0]['methods'] );
        $this->assertArrayNotHasKey( 'DELETE', $routes[$full_route][0]['methods'] );
        
        // Test invalid route
        $invalid_route = $this->get_route( 'data/invalid' );
        $this->assertArrayNotHasKey( $invalid_route, $routes );
    }
tests/php/src/REST/ShippingMethodControllerTest.php (2)

32-42: ⚠️ Potential issue

Expand test coverage for shipping methods retrieval

The current test only covers the happy path. Consider adding:

  1. Tests for different user roles (guest, customer, admin)
  2. Validation of response structure and required fields
  3. Error cases (e.g., invalid authentication)
     public function test_get_shipping_methods() {
-        wp_set_current_user( $this->seller_id1 );
+        $test_cases = [
+            'guest' => [
+                'user_id' => 0,
+                'expected_status' => 401
+            ],
+            'customer' => [
+                'user_id' => $this->customer,
+                'expected_status' => 403
+            ],
+            'seller' => [
+                'user_id' => $this->seller_id1,
+                'expected_status' => 200
+            ]
+        ];
 
-        $response = $this->get_request( 'shipping_methods' );
+        foreach ($test_cases as $case => $config) {
+            wp_set_current_user($config['user_id']);
+            $response = $this->get_request('shipping_methods');
+            $this->assertEquals(
+                $config['expected_status'],
+                $response->get_status(),
+                "Failed asserting response status for $case user"
+            );
 
-        $this->assertEquals( 200, $response->get_status() );
-        $data = $response->get_data();
-        $this->assertIsArray( $data );
-        $this->assertNotEmpty( $data );
-        // Add more assertions based on expected shipping method data
+            if ($config['expected_status'] === 200) {
+                $data = $response->get_data();
+                $this->assertIsArray($data);
+                $this->assertNotEmpty($data);
+                
+                // Verify required fields for each shipping method
+                foreach ($data as $method) {
+                    $this->assertArrayHasKey('id', $method);
+                    $this->assertArrayHasKey('title', $method);
+                    $this->assertArrayHasKey('enabled', $method);
+                    $this->assertIsBool($method['enabled']);
+                }
+            }
+        }
     }

Committable suggestion was skipped due to low confidence.


47-56: ⚠️ Potential issue

Improve single shipping method test robustness

Current implementation has several limitations:

  1. Relies on 'flat_rate' existing without proper setup
  2. Lacks error case testing
  3. Minimal response validation
     public function test_get_shipping_method() {
-        wp_set_current_user( $this->seller_id1 );
+        // Setup
+        $expected_methods = [
+            'flat_rate' => [
+                'id' => 'flat_rate',
+                'title' => 'Flat Rate',
+                'enabled' => true
+            ]
+        ];
+        add_filter('woocommerce_shipping_methods', function($methods) use ($expected_methods) {
+            return array_merge($methods, $expected_methods);
+        });
 
-        // Assuming 'flat_rate' is a valid shipping method ID
-        $response = $this->get_request( 'shipping_methods/flat_rate' );
+        $test_cases = [
+            'invalid_id' => [
+                'method_id' => 'non_existent',
+                'expected_status' => 404
+            ],
+            'valid_id' => [
+                'method_id' => 'flat_rate',
+                'expected_status' => 200
+            ]
+        ];
 
-        $this->assertEquals( 200, $response->get_status() );
-        $data = $response->get_data();
-        $this->assertEquals( 'flat_rate', $data['id'] );
+        wp_set_current_user($this->seller_id1);
+
+        foreach ($test_cases as $case => $config) {
+            $response = $this->get_request("shipping_methods/{$config['method_id']}");
+            
+            $this->assertEquals(
+                $config['expected_status'],
+                $response->get_status(),
+                "Failed asserting response status for $case"
+            );
+
+            if ($config['expected_status'] === 200) {
+                $data = $response->get_data();
+                $expected = $expected_methods[$config['method_id']];
+                
+                $this->assertEquals($expected['id'], $data['id']);
+                $this->assertEquals($expected['title'], $data['title']);
+                $this->assertEquals($expected['enabled'], $data['enabled']);
+            }
+        }
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    public function test_get_shipping_method() {
        // Setup
        $expected_methods = [
            'flat_rate' => [
                'id' => 'flat_rate',
                'title' => 'Flat Rate',
                'enabled' => true
            ]
        ];
        add_filter('woocommerce_shipping_methods', function($methods) use ($expected_methods) {
            return array_merge($methods, $expected_methods);
        });

        $test_cases = [
            'invalid_id' => [
                'method_id' => 'non_existent',
                'expected_status' => 404
            ],
            'valid_id' => [
                'method_id' => 'flat_rate',
                'expected_status' => 200
            ]
        ];

        wp_set_current_user($this->seller_id1);

        foreach ($test_cases as $case => $config) {
            $response = $this->get_request("shipping_methods/{$config['method_id']}");
            
            $this->assertEquals(
                $config['expected_status'],
                $response->get_status(),
                "Failed asserting response status for $case"
            );

            if ($config['expected_status'] === 200) {
                $data = $response->get_data();
                $expected = $expected_methods[$config['method_id']];
                
                $this->assertEquals($expected['id'], $data['id']);
                $this->assertEquals($expected['title'], $data['title']);
                $this->assertEquals($expected['enabled'], $data['enabled']);
            }
        }
    }
includes/REST/TaxController.php (3)

11-18: ⚠️ Potential issue

Replace "DOKAN_SINCE" placeholder with actual version number.

The @since tag contains a placeholder that needs to be replaced with the actual version number where this class is introduced.

- * @since DOKAN_SINCE
+ * @since 3.9.0
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

/**
 * Dokan REST API Taxes controller class.
 *
 * @since 3.9.0
 *
 * @package WeDevs\Dokan\REST
 */
class TaxController extends WC_REST_Taxes_Controller {

53-60: 🛠️ Refactor suggestion

Enhance permission checks with specific capabilities.

The current vendor permission check only verifies if the user is a seller. For tax management, consider adding specific capability checks to ensure the vendor has the required permissions to manage tax settings.

 protected function check_vendor_permission(): bool {
-    return dokan_is_user_seller( dokan_get_current_user_id() );
+    $user_id = dokan_get_current_user_id();
+    return dokan_is_user_seller( $user_id ) && 
+           user_can( $user_id, 'dokan_manage_tax' );
 }

This ensures that vendors not only need to be sellers but also have the specific capability to manage taxes.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    /**
     * Check vendor permission.
     *
     * @return bool
     */
    protected function check_vendor_permission(): bool {
        $user_id = dokan_get_current_user_id();
        return dokan_is_user_seller( $user_id ) && 
               user_can( $user_id, 'dokan_manage_tax' );
    }

27-51: 💡 Codebase verification

⚠️ Potential issue

Based on the search results, I can now provide a final response as we have sufficient information about permission check implementations across various controllers in the codebase.

Permission checks for write operations are required for consistency with other controllers

Looking at other controllers in the codebase, particularly OrderNoteControllerV3, OrderRefundControllerV3, and OrderControllerV3, they implement full CRUD permission checks. The TaxController should follow the same pattern for consistency. Specifically:

  • create_item_permissions_check: Implemented in OrderNoteControllerV3, OrderRefundControllerV3, OrderControllerV3
  • update_item_permissions_check: Implemented in OrderControllerV3
  • delete_item_permissions_check: Implemented in OrderRefundControllerV3, OrderControllerV3

The implementation should follow the same pattern as other controllers:

public function create_item_permissions_check($request) {
    if (!$this->check_vendor_permission()) {
        return new WP_Error('dokan_rest_cannot_create', __('Sorry, you are not allowed to create resources.', 'dokan-lite'), array('status' => rest_authorization_required_code()));
    }
    return true;
}
🔗 Analysis chain

Implement permission checks for write operations.

The current implementation only includes permission checks for reading tax resources (get_items and get_item). According to the PR objectives, CRUD operations should be supported. Please implement permission checks for create, update, and delete operations.

Consider adding these methods:

  • create_item_permissions_check
  • update_item_permissions_check
  • delete_item_permissions_check

Let's verify if other controllers implement these permission checks:


Let me try a different approach to search for permission check methods in the controllers.

This should help us understand:

  1. What controller files exist in the codebase
  2. What permission check methods are implemented in these controllers
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for create/update/delete permission checks in other controllers
ast-grep --pattern 'function $_(create|update|delete)_item_permissions_check'

Length of output: 79


Script:

#!/bin/bash
# Search for controller files
fd "Controller\.php$" --type f

# Then search for permission check methods in these files
rg "function.*_permissions_check" --type php -A 5

Length of output: 31481

includes/REST/ShippingMethodController.php (3)

11-18: ⚠️ Potential issue

Replace DOKAN_SINCE placeholder with actual version.

The @since DOKAN_SINCE tag needs to be replaced with the actual version number where this class was introduced.


58-60: 🛠️ Refactor suggestion

Consider caching the vendor permission check result.

Since the vendor status is unlikely to change during a single request, consider caching the result to avoid multiple database queries:

+    /**
+     * Cached result of vendor permission check.
+     *
+     * @var bool|null
+     */
+    private ?bool $is_vendor = null;

     protected function check_vendor_permission(): bool {
-        return dokan_is_user_seller( dokan_get_current_user_id() );
+        if ( $this->is_vendor === null ) {
+            $this->is_vendor = dokan_is_user_seller( dokan_get_current_user_id() );
+        }
+        return $this->is_vendor;
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    /**
     * Cached result of vendor permission check.
     *
     * @var bool|null
     */
    private ?bool $is_vendor = null;

    protected function check_vendor_permission(): bool {
        if ( $this->is_vendor === null ) {
            $this->is_vendor = dokan_is_user_seller( dokan_get_current_user_id() );
        }
        return $this->is_vendor;
    }

33-51: 🛠️ Refactor suggestion

Consider reducing code duplication in permission checks.

The permission check methods share almost identical code. Consider extracting the common logic into a reusable method:

+    /**
+     * Handle common permission check logic.
+     *
+     * @param string $message Error message for unauthorized access.
+     * @return WP_Error|boolean
+     */
+    protected function handle_permission_check( string $message ): WP_Error|bool {
+        if ( ! $this->check_vendor_permission() ) {
+            return new WP_Error(
+                'dokan_rest_cannot_view',
+                $message,
+                array( 'status' => rest_authorization_required_code() )
+            );
+        }
+        return true;
+    }

     public function get_items_permissions_check( $request ) {
-        if ( ! $this->check_vendor_permission() ) {
-            return new WP_Error( 'dokan_rest_cannot_view', __( 'Sorry, you cannot list resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) );
-        }
-        return true;
+        return $this->handle_permission_check( __( 'Sorry, you cannot list resources.', 'dokan-lite' ) );
     }

     public function get_item_permissions_check( $request ) {
-        if ( ! $this->check_vendor_permission() ) {
-            return new WP_Error( 'dokan_rest_cannot_view', __( 'Sorry, you cannot view this resource.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) );
-        }
-        return true;
+        return $this->handle_permission_check( __( 'Sorry, you cannot view this resource.', 'dokan-lite' ) );
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    /**
     * Handle common permission check logic.
     *
     * @param string $message Error message for unauthorized access.
     * @return WP_Error|boolean
     */
    protected function handle_permission_check( string $message ): WP_Error|bool {
        if ( ! $this->check_vendor_permission() ) {
            return new WP_Error(
                'dokan_rest_cannot_view',
                $message,
                array( 'status' => rest_authorization_required_code() )
            );
        }
        return true;
    }

    public function get_items_permissions_check( $request ) {
        return $this->handle_permission_check( __( 'Sorry, you cannot list resources.', 'dokan-lite' ) );
    }

    /**
     * Check if a given request has access to read a shipping method.
     *
     * @param  WP_REST_Request $request Full details about the request.
     * @return WP_Error|boolean
     */
    public function get_item_permissions_check( $request ) {
        return $this->handle_permission_check( __( 'Sorry, you cannot view this resource.', 'dokan-lite' ) );
    }
includes/REST/PaymentGatewayController.php (1)

33-38: 🛠️ Refactor suggestion

Consider improving error messages and reducing code duplication.

The permission check methods could be enhanced:

  1. Error messages could be more specific about the required vendor permissions
  2. Consider extracting the common logic to reduce duplication
  3. Add @return tags in docblocks
 /**
  * Check whether a given request has permission to read payment gateways.
  *
  * @param  WP_REST_Request $request Full details about the request.
+ * @return WP_Error|boolean WP_Error if user lacks permission, true otherwise.
  */
 public function get_items_permissions_check( $request ) {
     if ( ! $this->check_vendor_permission() ) {
-        return new WP_Error( 'dokan_rest_cannot_view', __( 'Sorry, you cannot list resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) );
+        return new WP_Error(
+            'dokan_rest_cannot_view',
+            __( 'Sorry, you need vendor payment menu access permissions to view payment gateways.', 'dokan-lite' ),
+            array( 'status' => rest_authorization_required_code() )
+        );
     }
     return true;
 }

Also applies to: 46-51

tests/php/src/REST/OrderActionsControllerV3Test.php (2)

18-71: 🛠️ Refactor suggestion

Consider adding more test scenarios for comprehensive coverage.

While the current tests cover basic functionality, consider adding tests for:

  1. Unauthorized access (non-seller trying to access actions)
  2. Non-existent order scenarios
  3. Different action types using a data provider
  4. Concurrent action execution edge cases

Example implementation for unauthorized access test:

/**
 * Test unauthorized access to order actions.
 */
public function test_get_order_actions_unauthorized() {
    wp_set_current_user( $this->customer );  // Set as regular customer

    $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create();
    $response = $this->get_request( "orders/{$order_id}/actions" );

    $this->assertEquals( 403, $response->get_status() );
}

28-31: 🛠️ Refactor suggestion

Strengthen assertions for order actions response structure.

The current assertions only check for key existence. Consider adding more specific assertions about the expected data structure and values.

 $this->assertIsArray( $data );
-$this->assertArrayHasKey( 'send_order_details', $data );
-$this->assertArrayHasKey( 'send_order_details_admin', $data );
-$this->assertArrayHasKey( 'regenerate_download_permissions', $data );
+$this->assertArrayHasKey( 'send_order_details', $data );
+$this->assertIsString( $data['send_order_details'] );
+$this->assertArrayHasKey( 'send_order_details_admin', $data );
+$this->assertIsString( $data['send_order_details_admin'] );
+$this->assertArrayHasKey( 'regenerate_download_permissions', $data );
+$this->assertIsString( $data['regenerate_download_permissions'] );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

        $this->assertIsArray( $data );
        $this->assertArrayHasKey( 'send_order_details', $data );
        $this->assertIsString( $data['send_order_details'] );
        $this->assertArrayHasKey( 'send_order_details_admin', $data );
        $this->assertIsString( $data['send_order_details_admin'] );
        $this->assertArrayHasKey( 'regenerate_download_permissions', $data );
        $this->assertIsString( $data['regenerate_download_permissions'] );
tests/php/src/REST/OrderRefundsControllerV3Test.php (2)

19-67: 🛠️ Refactor suggestion

Consider expanding test coverage for comprehensive validation.

While the happy path is well tested, consider adding the following test scenarios:

  1. Error cases (invalid order ID, insufficient permissions)
  2. Validation of all refund response properties
  3. Business rules validation (e.g., refund amount cannot exceed order total)

Here's a suggested approach for additional test methods:

public function test_create_refund_with_invalid_order_id() {
    wp_set_current_user($this->seller_id1);
    
    $response = $this->post_request('orders/999999/refunds', [
        'amount' => 10,
        'reason' => 'testing'
    ]);
    
    $this->assertEquals(404, $response->get_status());
}

public function test_create_refund_exceeding_order_total() {
    wp_set_current_user($this->seller_id1);
    
    $order_id = $this->factory()->order->create_order_with_fees_and_shipping();
    $order = wc_get_order($order_id);
    $total = $order->get_total();
    
    $response = $this->post_request("orders/{$order_id}/refunds", [
        'amount' => $total + 100,
        'reason' => 'testing'
    ]);
    
    $this->assertEquals(400, $response->get_status());
}

54-66: 🛠️ Refactor suggestion

Enhance response validation.

The current assertions only check for the presence of keys and total values. Consider validating all relevant properties of the refund response.

     $response = $this->get_request('orders/' . $order->get_id() . '/refunds/' . $refund->get_id());
     $data = $response->get_data();
 
     $this->assertEquals(200, $response->get_status());
 
+    // Validate common refund properties
+    $expected_properties = ['id', 'date_created', 'reason', 'refunded_by'];
+    foreach ($expected_properties as $property) {
+        $this->assertArrayHasKey($property, $data);
+    }
+
     // Validate line items
     $this->assertContains('line_items', array_keys($data));
     $this->assertEquals(-1, $data['line_items'][0]['total']);
+    $this->assertEquals(1, $data['line_items'][0]['quantity']);
+    $this->assertArrayHasKey('name', $data['line_items'][0]);
 
     // Validate fee lines
     $this->assertContains('fee_lines', array_keys($data));
     $this->assertEquals(-10, $data['fee_lines'][0]['total']);
+    $this->assertArrayHasKey('name', $data['fee_lines'][0]);
 
     // Validate shipping lines
     $this->assertContains('shipping_lines', array_keys($data));
     $this->assertEquals(-20, $data['shipping_lines'][0]['total']);
+    $this->assertArrayHasKey('method_title', $data['shipping_lines'][0]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

        $response = $this->get_request( 'orders/' . $order->get_id() . '/refunds/' . $refund->get_id() );
        $data = $response->get_data();

        $this->assertEquals( 200, $response->get_status() );

        // Validate common refund properties
        $expected_properties = ['id', 'date_created', 'reason', 'refunded_by'];
        foreach ($expected_properties as $property) {
            $this->assertArrayHasKey($property, $data);
        }

        // Validate line items
        $this->assertContains( 'line_items', array_keys( $data ) );
        $this->assertEquals( -1, $data['line_items'][0]['total'] );
        $this->assertEquals(1, $data['line_items'][0]['quantity']);
        $this->assertArrayHasKey('name', $data['line_items'][0]);

        // Validate fee lines
        $this->assertContains( 'fee_lines', array_keys( $data ) );
        $this->assertEquals( -10, $data['fee_lines'][0]['total'] );
        $this->assertArrayHasKey('name', $data['fee_lines'][0]);

        // Validate shipping lines
        $this->assertContains( 'shipping_lines', array_keys( $data ) );
        $this->assertEquals( -20, $data['shipping_lines'][0]['total'] );
        $this->assertArrayHasKey('method_title', $data['shipping_lines'][0]);
tests/php/src/REST/OrderNotesControllerTestV3.php (4)

15-36: 🛠️ Refactor suggestion

Add test coverage for edge cases in order note creation.

The current test only covers the happy path. Consider adding test cases for:

  1. Unauthorized access (non-seller trying to add notes)
  2. Invalid order ID
  3. Empty or invalid note content

Add these test methods:

public function test_create_order_note_unauthorized() {
    wp_set_current_user( $this->customer );
    $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create();
    
    $response = $this->post_request(
        "orders/{$order_id}/notes",
        ['note' => 'Test note']
    );
    
    $this->assertEquals( 403, $response->get_status() );
}

public function test_create_order_note_invalid_order() {
    wp_set_current_user( $this->seller_id1 );
    
    $response = $this->post_request(
        "orders/999999/notes",
        ['note' => 'Test note']
    );
    
    $this->assertEquals( 404, $response->get_status() );
}

public function test_create_order_note_empty_content() {
    wp_set_current_user( $this->seller_id1 );
    $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create();
    
    $response = $this->post_request(
        "orders/{$order_id}/notes",
        ['note' => '']
    );
    
    $this->assertEquals( 400, $response->get_status() );
}

11-14: 🛠️ Refactor suggestion

Consider adding a setUp method to reduce code duplication.

Each test method starts with similar setup code. Consider extracting this to a setUp method:

protected $order_id;

protected function setUp(): void {
    parent::setUp();
    wp_set_current_user( $this->seller_id1 );
    $this->order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create();
}

This would make the test methods more focused and reduce duplication.


41-60: ⚠️ Potential issue

Make note order assertions more robust.

The current test assumes a specific order of notes in the response. Since the order might not be guaranteed, consider either:

  1. Adding an ORDER BY clause in the controller
  2. Making the assertions order-independent

Here's how to make the assertions more robust:

-        $this->assertEquals( 'Note 1', $data[0]['note'] );
-        $this->assertFalse( $data[0]['customer_note'] );
-        $this->assertEquals( 'Note 2', $data[1]['note'] );
-        $this->assertTrue( $data[1]['customer_note'] );
+        $notes = array_column($data, 'note');
+        $this->assertContains('Note 1', $notes);
+        $this->assertContains('Note 2', $notes);
+        
+        $note1 = current(array_filter($data, function($note) {
+            return $note['note'] === 'Note 1';
+        }));
+        $note2 = current(array_filter($data, function($note) {
+            return $note['note'] === 'Note 2';
+        }));
+        
+        $this->assertFalse($note1['customer_note']);
+        $this->assertTrue($note2['customer_note']);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    public function test_get_order_notes() {
        wp_set_current_user( $this->seller_id1 );

        $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create();

        // Add some notes
        wc_create_order_note( $order_id, 'Note 1' );
        wc_create_order_note( $order_id, 'Note 2', 1, true );

        $response = $this->get_request( "orders/{$order_id}/notes" );

        $this->assertEquals( 200, $response->get_status() );
        $data = $response->get_data();

        $this->assertCount( 2, $data );
        $notes = array_column($data, 'note');
        $this->assertContains('Note 1', $notes);
        $this->assertContains('Note 2', $notes);
        
        $note1 = current(array_filter($data, function($note) {
            return $note['note'] === 'Note 1';
        }));
        $note2 = current(array_filter($data, function($note) {
            return $note['note'] === 'Note 2';
        }));
        
        $this->assertFalse($note1['customer_note']);
        $this->assertTrue($note2['customer_note']);
    }

65-80: 🛠️ Refactor suggestion

Enhance delete note test coverage and documentation.

The test should validate the response data structure and handle edge cases. Also, document the purpose of the force parameter.

     public function test_delete_order_note() {
         wp_set_current_user( $this->seller_id1 );
 
         $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create();
         $note_id = wc_create_order_note( $order_id, 'Test note' );
 
         $response = $this->delete_request(
             "orders/{$order_id}/notes/{$note_id}",
             array( 'force' => true )
         );
 
         $this->assertEquals( 200, $response->get_status() );
+        $data = $response->get_data();
+        
+        // Verify response structure
+        $this->assertArrayHasKey('deleted', $data);
+        $this->assertTrue($data['deleted']);
 
         // Verify the note is deleted
         $this->assertEmpty( wc_get_order_notes( [ 'id' => $note_id ] ) );
+        
+        // Verify deleting again returns 404
+        $response = $this->delete_request(
+            "orders/{$order_id}/notes/{$note_id}",
+            array( 'force' => true )
+        );
+        $this->assertEquals( 404, $response->get_status() );
     }

Also add a docblock explaining the force parameter:

     /**
      * Test deleting an order note.
+     * 
+     * @since 3.0.0
+     * 
+     * The 'force' parameter is used to permanently delete the note
+     * instead of moving it to trash. In WP REST API, this is a 
+     * common pattern for delete operations.
      */
     public function test_delete_order_note() {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    /**
     * Test deleting an order note.
     * 
     * @since 3.0.0
     * 
     * The 'force' parameter is used to permanently delete the note
     * instead of moving it to trash. In WP REST API, this is a 
     * common pattern for delete operations.
     */
    public function test_delete_order_note() {
        wp_set_current_user( $this->seller_id1 );

        $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create();
        $note_id = wc_create_order_note( $order_id, 'Test note' );

        $response = $this->delete_request(
            "orders/{$order_id}/notes/{$note_id}",
            array( 'force' => true )
        );

        $this->assertEquals( 200, $response->get_status() );
        $data = $response->get_data();
        
        // Verify response structure
        $this->assertArrayHasKey('deleted', $data);
        $this->assertTrue($data['deleted']);

        // Verify the note is deleted
        $this->assertEmpty( wc_get_order_notes( [ 'id' => $note_id ] ) );
        
        // Verify deleting again returns 404
        $response = $this->delete_request(
            "orders/{$order_id}/notes/{$note_id}",
            array( 'force' => true )
        );
        $this->assertEquals( 404, $response->get_status() );
    }
includes/REST/Manager.php (1)

237-246: 💡 Codebase verification

Now I have enough information to provide the final response:

All controller files are present except ShippingStatusController.php

The verification shows that 9 out of 10 controller files exist in the codebase:

  • DataCountryController.php
  • ShippingMethodController.php
  • TaxClassController.php
  • TaxController.php
  • PaymentGatewayController.php
  • OrderControllerV3.php
  • OrderNoteControllerV3.php
  • OrderRefundControllerV3.php
  • OrderActionControllerV3.php

However, ShippingStatusController.php is missing from the codebase despite being referenced in the Manager.php class map. This needs to be addressed.

🔗 Analysis chain

Verify existence of new controller files.

Let's ensure all new controller files are present in the codebase.


Let me analyze the results. The output shows that only 4 out of 10 controller files were found in the codebase:

  • OrderControllerV3.php
  • OrderNoteControllerV3.php
  • OrderRefundControllerV3.php
  • OrderActionControllerV3.php

However, let me verify with a different search strategy to ensure we're not missing any files due to path specifics.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if all new controller files exist

# Test: Check existence of new controller files
for controller in DataCountry ShippingMethod ShippingStatus TaxClass Tax PaymentGateway OrderControllerV3 OrderNoteControllerV3 OrderRefundControllerV3 OrderActionControllerV3; do
    fd -t f "${controller}.php$" -E "vendor/*"
done

Length of output: 2407


Script:

#!/bin/bash
# Search in includes/REST directory specifically since that's where the files should be
ls -la includes/REST/DataCountry*.php includes/REST/ShippingMethod*.php includes/REST/ShippingStatus*.php includes/REST/TaxClass*.php includes/REST/Tax*.php includes/REST/PaymentGateway*.php 2>/dev/null

# Also do a broader search with ripgrep to find any references to these controllers
rg -l "class (DataCountry|ShippingMethod|ShippingStatus|TaxClass|Tax|PaymentGateway)Controller" --type php

Length of output: 1245

includes/REST/TaxClassController.php (1)

14-14: ⚠️ Potential issue

Replace placeholder 'DOKAN_SINCE' with actual version number in @since tag

The @since annotation in the class documentation uses the placeholder DOKAN_SINCE. Please replace it with the actual version number of the plugin in which this class was introduced.

includes/REST/OrderRefundControllerV3.php (7)

14-14: ⚠️ Potential issue

Replace placeholder DOKAN_SINCE with the actual version number

The @since DOKAN_SINCE tag in the class documentation should be updated to reflect the actual version number before release.


75-85: ⚠️ Potential issue

Handle invalid refund IDs gracefully

In get_item_permissions_check, after retrieving the refund object, you check if it exists. Ensure that if the refund does not exist or the parent_id is invalid, appropriate errors are returned.


95-105: ⚠️ Potential issue

Validate refund existence before authorization in delete_item_permissions_check

Ensure that the refund object is valid before proceeding with order authorization checks to prevent errors when get_parent_id() is called on a non-existent refund.


33-114: 🛠️ Refactor suggestion

Consider adding capability checks for enhanced security

While you are checking if the current user is a seller, you might also want to check if they have specific capabilities required for refund operations to ensure robust security.


33-39: 🛠️ Refactor suggestion

Use consistent status codes and messages in authorization checks

In the check_order_authorization method, you return a WP_Error with status code 403. Consider using rest_authorization_required_code() instead of hardcoding 403 to maintain consistency with WordPress REST API practices.

Apply this diff to use the consistent authorization code:

 return new WP_Error(
     'dokan_rest_unauthorized_order',
     __( 'You do not have permission to access this order', 'dokan-lite' ),
-    array( 'status' => 403 )
+    array( 'status' => rest_authorization_required_code() )
 );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    protected function check_order_authorization( int $order_id ) {
        $vendor_id = dokan_get_seller_id_by_order( $order_id );
        if ( $vendor_id !== dokan_get_current_user_id() ) {
            return new WP_Error( 'dokan_rest_unauthorized_order', __( 'You do not have permission to access this order', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) );
        }
        return true;
    }

61-67: ⚠️ Potential issue

Ensure order_id validation in create_item_permissions_check

Similar to get_items_permissions_check, make sure that order_id is validated in create_item_permissions_check to prevent potential issues.

Apply this diff to add order_id validation:

 public function create_item_permissions_check( $request ) {
     if ( ! $this->check_vendor_permission() ) {
         return new WP_Error(
             'dokan_rest_cannot_create',
             __( 'Sorry, you are not allowed to create resources.', 'dokan-lite' ),
             array( 'status' => rest_authorization_required_code() )
         );
     }
+    if ( empty( $request['order_id'] ) ) {
+        return new WP_Error(
+            'dokan_rest_invalid_order_id',
+            __( 'Order ID is required.', 'dokan-lite' ),
+            array( 'status' => 400 )
+        );
+    }
     return $this->check_order_authorization( $request['order_id'] );
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    public function create_item_permissions_check( $request ) {
        if ( ! $this->check_vendor_permission() ) {
            return new WP_Error(
                'dokan_rest_cannot_create',
                __( 'Sorry, you are not allowed to create resources.', 'dokan-lite' ),
                array( 'status' => rest_authorization_required_code() )
            );
        }
        if ( empty( $request['order_id'] ) ) {
            return new WP_Error(
                'dokan_rest_invalid_order_id',
                __( 'Order ID is required.', 'dokan-lite' ),
                array( 'status' => 400 )
            );
        }
        return $this->check_order_authorization( $request['order_id'] );
    }

47-53: ⚠️ Potential issue

Validate the presence of order_id in the request

In get_items_permissions_check, ensure that order_id is provided in the request to avoid undefined index notices or errors.

Apply this diff to validate order_id:

 public function get_items_permissions_check( $request ) {
     if ( ! $this->check_vendor_permission() ) {
         return new WP_Error(
             'dokan_rest_cannot_view',
             __( 'Sorry, you cannot list resources.', 'dokan-lite' ),
             array( 'status' => rest_authorization_required_code() )
         );
     }
+    if ( empty( $request['order_id'] ) ) {
+        return new WP_Error(
+            'dokan_rest_invalid_order_id',
+            __( 'Order ID is required.', 'dokan-lite' ),
+            array( 'status' => 400 )
+        );
+    }
     return $this->check_order_authorization( $request['order_id'] );
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    public function get_items_permissions_check( $request ) {
        if ( ! $this->check_vendor_permission() ) {
            return new WP_Error(
                'dokan_rest_cannot_view',
                __( 'Sorry, you cannot list resources.', 'dokan-lite' ),
                array( 'status' => rest_authorization_required_code() )
            );
        }
        if ( empty( $request['order_id'] ) ) {
            return new WP_Error(
                'dokan_rest_invalid_order_id',
                __( 'Order ID is required.', 'dokan-lite' ),
                array( 'status' => 400 )
            );
        }
        return $this->check_order_authorization( $request['order_id'] );
    }
includes/REST/OrderNoteControllerV3.php (4)

115-131: ⚠️ Potential issue

Handle Unexpected Response Types in create_item

In the create_item method, after calling parent::create_item( $request ), consider checking if $response is an instance of WP_REST_Response before proceeding to use its methods to prevent potential errors.

Apply this check:

if ( is_wp_error( $response ) ) {
    return $response;
}

if ( ! ( $response instanceof WP_REST_Response ) ) {
    return new WP_Error( 'dokan_rest_invalid_response', esc_html__( 'Invalid response from parent method.', 'dokan-lite' ), array( 'status' => 500 ) );
}

if ( $response->get_status() === 201 ) {
    // Proceed with updating the note author
}

146-158: 🛠️ Refactor suggestion

Improve Data Handling in update_note_author

Consider validating and sanitizing user data before updating the comment to enhance security and data integrity.

Implement data sanitization:

if ( $creating && $user instanceof \WP_User && dokan_is_user_seller( $user_id ) ) {
    $data = array(
        'comment_ID'           => $note->comment_ID,
        'user_id'              => $user_id,
        'comment_author'       => sanitize_text_field( $user->display_name ),
        'comment_author_email' => sanitize_email( $user->user_email ),
    );

    wp_update_comment( wp_slash( $data ) );

    // Update comment meta to indicate it's a vendor note
    update_comment_meta( (int) $note->comment_ID, 'dokan_vendor_note', 1 );
}

93-93: ⚠️ Potential issue

Escape Error Messages in delete_item_permissions_check

For consistency and security, use esc_html__ in the delete_item_permissions_check method.

Apply this diff:

-return new WP_Error( 'dokan_rest_cannot_delete', __( 'Sorry, you are not allowed to delete this resource.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) );
+return new WP_Error( 'dokan_rest_cannot_delete', esc_html__( 'Sorry, you are not allowed to delete this resource.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

            return new WP_Error( 'dokan_rest_cannot_delete', esc_html__( 'Sorry, you are not allowed to delete this resource.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) );

51-51: ⚠️ Potential issue

Escape Error Messages for Security

In get_items_permissions_check, consider using esc_html__ instead of __ to properly escape the error message and enhance security by preventing potential HTML injection.

Apply this diff to address the issue:

-return new WP_Error( 'dokan_rest_cannot_view', __( 'Sorry, you cannot list resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) );
+return new WP_Error( 'dokan_rest_cannot_view', esc_html__( 'Sorry, you cannot list resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

            return new WP_Error( 'dokan_rest_cannot_view', esc_html__( 'Sorry, you cannot list resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) );
includes/REST/OrderActionControllerV3.php (4)

66-83: 🛠️ Refactor suggestion

Add return type declarations for consistency and type safety

Consider adding return type declarations to your methods to enhance code clarity and enforce type safety. This will help developers understand the expected return values and can prevent potential bugs.

Apply these diffs to add return type declarations:

- protected function check_order_authorization( int $order_id ) {
+ protected function check_order_authorization( int $order_id ): bool|\WP_Error {
- public function get_items_permissions_check( $request ) {
+ public function get_items_permissions_check( $request ): bool|\WP_Error {
- public function create_item_permissions_check( $request ) {
+ public function create_item_permissions_check( $request ): bool|\WP_Error {
- public function get_items( $request ) {
+ public function get_items( $request ): WP_REST_Response|\WP_Error {
- public function create_item( $request ) {
+ public function create_item( $request ): WP_REST_Response|\WP_Error {
- private function process_order_action( WC_Order $order, string $action ) {
+ private function process_order_action( WC_Order $order, string $action ): bool|\WP_Error {

Also applies to: 91-97, 105-111, 128-139, 148-170, 206-299


206-299: 🛠️ Refactor suggestion

Handle unexpected actions more gracefully in process_order_action

When an unrecognized action is received, consider providing more context in the error message to aid in debugging. Logging the unexpected action could also help in monitoring and resolving potential issues.


155-155: ⚠️ Potential issue

Sanitize the 'action' parameter from the request

Ensure that the 'action' parameter retrieved from the request is properly sanitized to prevent potential security issues related to untrusted input.

Apply this diff to sanitize the 'action' parameter:

- $action = $request['action'];
+ $action = sanitize_text_field( $request['action'] );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

        $action = sanitize_text_field( $request['action'] );

278-296: ⚠️ Potential issue

Use has_action instead of did_action to check for custom order actions

In the process_order_action method, the default case uses did_action() to check if a custom order action exists. However, did_action() checks how many times an action has been fired, not whether it's registered. To verify if an action hook exists, you should use has_action() instead.

Apply this diff to fix the issue:

- if ( did_action( 'woocommerce_order_action_' . sanitize_title( $action ) ) ) {
+ if ( has_action( 'woocommerce_order_action_' . sanitize_title( $action ) ) ) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

            default:
                if ( has_action( 'woocommerce_order_action_' . sanitize_title( $action ) ) ) {
                    /**
                     * Fires when a custom order action is being processed.
                     *
                     * This hook allows third-party plugins to add custom order actions
                     * and define their behavior when triggered through the API.
                     *
                     * @since DOKAN_SINCE
                     *
                     * @param WC_Order $order The order object for which the action is being performed.
                     *
                     * @hook woocommerce_order_action_{$action}
                     */
                    do_action( 'woocommerce_order_action_' . sanitize_title( $action ), $order );
                } else {
                    return new WP_Error( 'dokan_rest_invalid_order_action', esc_html__( 'Invalid order action.', 'dokan-lite' ), array( 'status' => 400 ) );
                }
        }
includes/REST/OrderControllerV3.php (3)

175-180: 🛠️ Refactor suggestion

Optimize query performance by indexing _dokan_vendor_id meta key

The meta query in prepare_objects_query filters orders by _dokan_vendor_id, which can lead to slow queries on large datasets because meta queries are less efficient. To improve performance, consider adding a database index on the _dokan_vendor_id meta key.


306-311: ⚠️ Potential issue

Ensure filter is removed even if an exception occurs

In the perform_vendor_action method, if an exception is thrown within the $action() callable, the remove_filter call will not execute, leaving the filter attached and potentially affecting subsequent operations. To ensure the filter is always removed, use a try...finally block.

Apply this diff to fix the issue:

private function perform_vendor_action( callable $action ) {
    add_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
-   $result = $action();
-   remove_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
+   try {
+       $result = $action();
+   } finally {
+       remove_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
+   }
    return $result;
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    private function perform_vendor_action( callable $action ) {
        add_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
        try {
            $result = $action();
        } finally {
            remove_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
        }
        return $result;
    }

286-286: ⚠️ Potential issue

Rename parameter to avoid using reserved keyword "object"

In PHP 7 and later, object is a reserved keyword. Using $object as a parameter name can lead to syntax errors or unexpected behavior. Please rename the $object parameter to something like $data_object to avoid conflicts.

Apply this diff to fix the issue:

-public function prepare_object_for_response( $object, $request ): WP_REST_Response {
+public function prepare_object_for_response( $data_object, $request ): WP_REST_Response {

Also, update all references to $object within the method accordingly.

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 GitHub Check: Run PHPCS inspection

[warning] 286-286:
It is recommended not to use reserved keyword "object" as function parameter name. Found: $object

tests/php/src/REST/OrdersControllerV3Test.php (4)

91-105: 🛠️ Refactor suggestion

Add missing return type declarations for consistency

Some test methods lack return type declarations while others include : void. For consistency and clarity, consider adding : void to methods that do not return a value.

Also applies to: 221-245


250-285: 🛠️ Refactor suggestion

Refactor duplicated code in meta data tests

The methods test_collection_param_include_meta, test_collection_param_include_meta_empty, and test_collection_param_exclude_meta contain similar setup and assertion code. Refactoring the common code into helper methods will improve maintainability and reduce duplication.

Also applies to: 290-326, 331-366


655-656: ⚠️ Potential issue

Fix incorrect discount calculation in test

The expected total is calculated with a 5% discount (* 0.95), but the coupon provides a 10% discount. Update the calculation to reflect the correct discount rate.

Apply this diff to correct the calculation:

- $expected_total = (float) ( $product->get_price() * 2 ) * 0.95; // 10% discount
+ $expected_total = (float) ( $product->get_price() * 2 ) * 0.9; // 10% discount
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

        $expected_total = (float) ( $product->get_price() * 2 ) * 0.9; // 10% discount
        $this->assertEquals( $expected_total, $data['total'] );

570-575: ⚠️ Potential issue

Correct meta data access in line items

Within the loop over $order['line_items'], the assertions check 'meta_data' in $order instead of $line_item. To accurately verify each line item's meta data, update the references to $line_item.

Apply this diff to fix the assertions:

- $this->assertArrayHasKey( 'meta_data', $order );
- $this->assertGreaterThanOrEqual( 3, count( $order['meta_data'] ) );
+ $this->assertArrayHasKey( 'meta_data', $line_item );
+ $this->assertGreaterThanOrEqual( 3, count( $line_item['meta_data'] ) );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

        $this->assertArrayHasKey( 'line_items', $order );

        foreach ( $order['line_items'] as $line_item ) {
            $this->assertArrayHasKey( 'meta_data', $line_item );
            $this->assertGreaterThanOrEqual( 3, count( $line_item['meta_data'] ) );

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 48

🧹 Outside diff range and nitpick comments (21)
tests/php/src/REST/TaxClassControllerTest.php (1)

1-39: Missing test cases for critical functionality.

The test suite should include additional test methods for:

  1. Tax class creation (POST)
  2. Tax class updates (PUT)
  3. Tax class deletion (DELETE)
  4. Validation of tax class data
  5. Rate limits and quotas
  6. Integration with WooCommerce tax settings

Would you like me to provide a comprehensive test suite implementation covering these scenarios?

tests/php/src/REST/TaxControllerTest.php (1)

7-12: Enhance class documentation.

Consider adding more comprehensive PHPDoc for the test class, including:

  • A description of what aspects of the Tax API are being tested
  • Any prerequisites or setup requirements
  • The API version being tested
 /**
+ * Class TaxControllerTest
+ *
+ * Tests the v1 REST API endpoints for tax management, including:
+ * - Route registration
+ * - Tax data retrieval
+ * 
+ * @since 1.0.0
  * @group dokan-taxes
  */
includes/REST/DataCountryController.php (2)

11-18: Replace DOKAN_SINCE placeholder with actual version number.

The @since DOKAN_SINCE placeholder should be replaced with the actual version number where this feature is being introduced.

- * @since DOKAN_SINCE
+ * @since 3.9.0

33-38: Consider enhancing error message with more context.

While the permission check is secure, the error message could be more specific about the required role/permission.

- return new WP_Error( 'dokan_rest_cannot_view', __( 'Sorry, you cannot list resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) );
+ return new WP_Error(
+     'dokan_rest_cannot_view',
+     __( 'Sorry, you need to be a vendor to access country data.', 'dokan-lite' ),
+     array( 'status' => rest_authorization_required_code() )
+ );
tests/php/src/REST/DataCountriesControllerTest.php (1)

7-13: Consider enhancing class documentation.

While the @group annotation is helpful, consider adding a class-level PHPDoc that describes the purpose and scope of these tests.

 /**
+ * Class DataCountriesControllerTest
+ *
+ * Tests for the REST API endpoints handling country and state data retrieval.
+ * Covers endpoints under 'dokan/v1/data/countries'.
+ *
  * @group dokan-data-countries
  */
tests/php/src/REST/ShippingMethodControllerTest.php (1)

1-11: Add strict types declaration and class documentation.

Consider enhancing the file header with strict types declaration and adding comprehensive class-level documentation.

 <?php
+declare(strict_types=1);

 namespace WeDevs\Dokan\Test\REST;

 use WeDevs\Dokan\Test\DokanTestCase;

 /**
+ * Class ShippingMethodControllerTest
+ *
+ * Tests for shipping method REST API endpoints to ensure proper functionality
+ * of shipping method retrieval and route registration.
+ *
  * @group dokan-shipping-methods
  */
 class ShippingMethodControllerTest extends DokanTestCase {
includes/REST/TaxController.php (1)

18-61: Consider adding vendor-specific tax functionality.

The controller currently only implements permission checks while inheriting all functionality from WC_REST_Taxes_Controller. Consider whether vendor-specific tax features are needed, such as:

  • Filtering tax rates by vendor
  • Adding vendor-specific tax settings
  • Implementing vendor tax reports endpoints

This would make the controller more aligned with vendor-specific requirements mentioned in the PR objectives.

includes/REST/TaxClassController.php (2)

11-25: Replace DOKAN_SINCE placeholder with actual version number.

The documentation block contains a placeholder @since DOKAN_SINCE which should be replaced with the actual version number where this feature is being introduced.


1-61: Consider integrating with a centralized permission system.

Since this is part of a larger REST API enhancement, consider implementing a centralized permission management system that can be reused across all new controllers (Orders, Taxes, Shipping, Payment). This would ensure consistent permission checking logic and make it easier to maintain and modify permissions in the future.

Key considerations for a centralized system:

  1. Create a shared trait or base class for common permission checks
  2. Implement capability-based permissions that can be filtered
  3. Add support for custom roles and multi-vendor scenarios
includes/REST/ShippingMethodController.php (1)

5-7: Consider organizing use statements alphabetically.

While the current implementation works correctly, organizing use statements alphabetically improves readability and maintainability.

-use WC_REST_Shipping_Methods_Controller;
-use WP_Error;
-use WP_REST_Request;
+use WC_REST_Shipping_Methods_Controller;
+use WP_Error;
+use WP_REST_Request;
includes/REST/PaymentGatewayController.php (2)

33-38: Consider using more specific error messages.

While the current error messages are functional, they could be more specific to help users understand why they can't access payment gateway information.

- __( 'Sorry, you cannot list resources.', 'dokan-lite' )
+ __( 'Sorry, you do not have permission to view payment gateways.', 'dokan-lite' )

- __( 'Sorry, you cannot view this resource.', 'dokan-lite' )
+ __( 'Sorry, you do not have permission to view this payment gateway.', 'dokan-lite' )

Also applies to: 46-51


18-61: Consider implementing prepare_item_for_response method.

Since this is a vendor-specific endpoint, you might want to customize the response data to only include information relevant to vendors. Consider overriding the prepare_item_for_response method from the parent class to filter sensitive gateway settings.

Example implementation:

/**
 * Prepare a payment gateway for response.
 *
 * @param WC_Payment_Gateway $item Payment gateway instance.
 * @param WP_REST_Request    $request Request object.
 * @return WP_REST_Response
 */
public function prepare_item_for_response( $item, $request ) {
    $response = parent::prepare_item_for_response( $item, $request );
    $data = $response->get_data();
    
    // Remove sensitive data that vendors shouldn't see
    unset( $data['settings'] );
    // Add vendor-specific data if needed
    
    $response->set_data( $data );
    return $response;
}
tests/php/src/REST/OrderActionsControllerV3Test.php (1)

7-11: Enhance test class documentation.

Consider adding a class-level PHPDoc block with a description and the @covers annotation to explicitly indicate which class is being tested.

 /**
+ * Class OrderActionsControllerV3Test
+ *
+ * Tests for order actions REST API endpoints in Dokan plugin.
+ *
+ * @covers \WeDevs\Dokan\REST\OrderActionsControllerV3
  * @group dokan-orders-v3
  * @group dokan-order-actions-v3
  */
tests/php/src/REST/OrderRefundsControllerV3Test.php (1)

16-19: Enhance PHPDoc with expected response structure.

The method documentation should include details about the expected response format and the significance of negative totals in refunds.

 /**
  * Test if line, fees and shipping items are all included in refund response.
+ *
+ * @testdox Refund response should include line items, fees, and shipping with negative totals
+ *
+ * @return void
+ *
+ * Expected response structure:
+ * {
+ *   "line_items": [{"total": "-1.00", ...}],
+ *   "fee_lines": [{"total": "-10.00", ...}],
+ *   "shipping_lines": [{"total": "-20.00", ...}]
+ * }
  */
tests/php/src/REST/OrderNotesControllerTestV3.php (1)

1-81: Consider implementing a comprehensive test matrix.

While the current tests cover basic CRUD operations, consider implementing a comprehensive test matrix that includes:

  1. Permission levels (vendor, customer, admin, guest)
  2. Edge cases (empty values, invalid IDs, malformed requests)
  3. Business logic variations (note types, visibility settings)
  4. Response format consistency
  5. Error message validation

This will ensure the API behaves consistently and securely across all scenarios.

includes/REST/Manager.php (1)

237-246: Consider architectural improvements for better maintainability.

As the number of controllers grows, consider these architectural improvements:

  1. Group controllers by domain (Orders, Shipping, etc.) using sub-managers
  2. Implement a versioning strategy that allows multiple versions to coexist
  3. Consider lazy loading of controllers to improve performance

Example structure:

namespace WeDevs\Dokan\REST;

abstract class BaseManager {
    protected function registerControllers() {}
}

class OrderManager extends BaseManager {
    protected function registerControllers() {
        // Register only order-related controllers
    }
}

Would you like me to provide a detailed implementation plan for these architectural improvements?

includes/REST/OrderNoteControllerV3.php (2)

86-86: Correct grammatical error in method comment

The doc comment for delete_item_permissions_check contains a grammatical error.

Current:

* Check if a given request has access delete a order note.

Suggested correction:

* Check if a given request has access to delete an order note.


142-142: Clarify the method description in the doc comment

The description for update_note_author should more accurately reflect its functionality.

Current:

* Update a single order note.

Suggested:

* Update the author information of an order note after creation.

includes/REST/OrderActionControllerV3.php (1)

178-196: Ensure Proper Localization for Custom Order Actions

The method get_available_order_actions_for_order allows filtering of the $actions array via the woocommerce_order_actions filter. While default actions are localized, custom actions added by third-party plugins may not be. To maintain consistency, ensure that any additional actions added through the filter are properly localized using translation functions.

Consider adding a note in the documentation:

/**
 * Filters the list of available order actions.
 *
 * This filter allows you to add or remove order actions for the Dokan vendor dashboard.
 * Note: Ensure that custom actions are localized using translation functions.
 *
 * @since DOKAN_SINCE
 *
 * @param array    $actions The list of available order actions.
 * @param WC_Order $order   The order object.
 */
includes/REST/OrderControllerV3.php (1)

171-173: Redundant vendor check in query preparation

The check for ! dokan_is_user_seller( $user ) before returning $args might be redundant if non-vendor users are already restricted from accessing this endpoint. Evaluate if this condition is necessary.

tests/php/src/REST/OrdersControllerV3Test.php (1)

110-113: Address the TODO regarding meta queries

In the test_orders_get_all method, there's a TODO comment indicating that meta queries are not working in test cases. Consider resolving this issue to ensure comprehensive testing of meta query functionality in order retrieval.

Would you like assistance in addressing the meta query issue in the test cases?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 1179a83 and 18c57e1.

📒 Files selected for processing (19)
  • includes/REST/DataCountryController.php (1 hunks)
  • includes/REST/Manager.php (1 hunks)
  • includes/REST/OrderActionControllerV3.php (1 hunks)
  • includes/REST/OrderControllerV3.php (1 hunks)
  • includes/REST/OrderNoteControllerV3.php (1 hunks)
  • includes/REST/OrderRefundControllerV3.php (1 hunks)
  • includes/REST/PaymentGatewayController.php (1 hunks)
  • includes/REST/ShippingMethodController.php (1 hunks)
  • includes/REST/TaxClassController.php (1 hunks)
  • includes/REST/TaxController.php (1 hunks)
  • tests/php/src/REST/DataCountriesControllerTest.php (1 hunks)
  • tests/php/src/REST/OrderActionsControllerV3Test.php (1 hunks)
  • tests/php/src/REST/OrderNotesControllerTestV3.php (1 hunks)
  • tests/php/src/REST/OrderRefundsControllerV3Test.php (1 hunks)
  • tests/php/src/REST/OrdersControllerV3Test.php (1 hunks)
  • tests/php/src/REST/PaymentGatewaysControllerTest.php (1 hunks)
  • tests/php/src/REST/ShippingMethodControllerTest.php (1 hunks)
  • tests/php/src/REST/TaxClassControllerTest.php (1 hunks)
  • tests/php/src/REST/TaxControllerTest.php (1 hunks)
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
includes/REST/OrderControllerV3.php

[warning] 286-286:
It is recommended not to use reserved keyword "object" as function parameter name. Found: $object

🔇 Additional comments (24)
tests/php/src/REST/TaxClassControllerTest.php (1)

1-13: ⚠️ Potential issue

Namespace mismatch between test class and route.

The test class uses namespace 'dokan/v1' but is grouped under '@group dokan-tax-classes-v3'. This inconsistency could lead to confusion. Either update the namespace to v3 or clarify why v1 is being used for a v3 feature.

Let's verify the namespace usage across the codebase:

tests/php/src/REST/TaxControllerTest.php (1)

1-5: LGTM! File structure follows PSR-4 standards.

The namespace and use statements are properly organized.

includes/REST/DataCountryController.php (2)

1-9: LGTM! Well-structured file header with proper security check.

The namespace, imports, and security check are properly implemented following WordPress coding standards.


20-25: Verify API version alignment with PR objectives.

The PR objectives mention different versions for different endpoints (v3 for Orders, v1 for Tax/Shipping/Payment). Since this is a data/country controller, please confirm if v1 is the intended version for this endpoint.

✅ Verification successful

Based on the search results, I can now generate the final response as I have sufficient information to verify the API versioning concern.

API version (v1) is correctly aligned for the DataCountryController

The version dokan/v1 is consistent with other data and configuration-related endpoints in the codebase:

  • All data controllers use v1: DataCountryController, DummyDataController
  • All configuration controllers use v1: TaxController, ShippingMethodController, PaymentGatewayController
  • Only Order-related controllers have v2 and v3 versions, which aligns with the PR objectives
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other API version declarations to ensure consistency
rg -g '*.php' 'protected \$namespace = .dokan/v[0-9]+.' includes/REST/

Length of output: 1965

tests/php/src/REST/PaymentGatewaysControllerTest.php (2)

1-10: LGTM! Well-structured test class setup.

The class follows PHP best practices with proper namespace, inheritance, and test group annotation.


12-13: LGTM! Correct API version used.

The namespace 'dokan/v1' aligns with the PR objectives for payment gateway endpoints.

tests/php/src/REST/DataCountriesControllerTest.php (2)

1-6: LGTM! File structure follows PSR standards.


1-54: Verify integration with shipping zone functionality.

Since this endpoint provides country/state data for shipping zones (as mentioned in PR objectives), consider adding tests that verify the data format matches what's expected by the shipping zone management endpoints.

includes/REST/TaxController.php (3)

1-9: LGTM! File structure follows best practices.

The file structure follows WordPress coding standards with proper namespace, imports, and security measures.


53-60: Add documentation for helper functions and verify their existence.

The method uses Dokan helper functions. Let's add proper documentation and verify these functions exist.

     /**
      * Check vendor permission.
      *
+     * @uses dokan_is_user_seller() Checks if the user is a seller
+     * @uses dokan_get_current_user_id() Gets the current user ID
      *
      * @return bool
      */
✅ Verification successful

Let me gather more information about these helper functions to provide accurate documentation.


Documentation suggestion verified and can be improved further

Based on the implementation details found, here's a more accurate documentation suggestion:

     /**
      * Check vendor permission.
      *
+     * @uses dokan_is_user_seller() Checks if a user has the 'dokandar' capability
+     * @uses dokan_get_current_user_id() Gets the current user ID, handles vendor staff cases
      *
      * @return bool
      */

The helper functions exist in includes/functions.php with proper implementations:

  • dokan_get_current_user_id() handles special logic for vendor staff users
  • dokan_is_user_seller() checks for the 'dokandar' capability
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify existence of Dokan helper functions
# Expect: Function definitions or references in core files

# Search for helper function definitions
rg "function (dokan_is_user_seller|dokan_get_current_user_id)\b" --type php

# Search for documentation of these functions
rg "@function (dokan_is_user_seller|dokan_get_current_user_id)\b" --type php

Length of output: 285


Script:

#!/bin/bash
# Get the implementation of these helper functions with context
rg "function dokan_(is_user_seller|get_current_user_id)" -B 2 -A 6 includes/functions.php

Length of output: 547


20-25: Verify API versioning strategy consistency.

The namespace is set to 'dokan/v1' which aligns with the PR objectives for Tax Management APIs (v1). However, let's verify this is consistent with other new controllers.

✅ Verification successful

Let me try a different search strategy to verify the API versioning consistency.

The previous search might have failed due to escaping issues. This new approach should help us find both explicit namespace definitions and route registrations to verify the API versioning strategy.


Let me fix the shell script syntax and try one more time.

The previous script had quote matching issues. This version uses a more precise regex pattern and removes the problematic parentheses in the command.


Let me try one final approach with simpler patterns.

I've simplified the patterns to avoid any syntax issues with quotes and made the search more basic but still effective for our verification needs.


API versioning is consistent with the codebase patterns

The namespace dokan/v1 in TaxController.php is correct and consistent with the codebase versioning patterns:

  • All tax-related controllers use dokan/v1:

    • TaxController.php: v1
    • TaxClassController.php: v1
    • Other related endpoints like PaymentGatewayController.php and DataCountryController.php: v1
  • The codebase shows clear versioning patterns:

    • v1: Used for most basic features and new standalone features
    • v2: Used for upgraded features (e.g., StoreSettingControllerV2, WithdrawControllerV2)
    • v3: Specifically used for order-related endpoints (e.g., OrderControllerV3, OrderNoteControllerV3)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check API versioning consistency across new controllers
# Expect: All tax-related endpoints use v1, while orders use v3

# Search for namespace definitions in REST controllers
rg -B 1 -A 1 "protected \$namespace = '(dokan|wc)/" --type php

Length of output: 66


Script:

#!/bin/bash
# Search for namespace property definitions in PHP files
rg -B 2 -A 2 '\$namespace\s*=\s*["\'].*["\']' --type php

# Also search for any route registrations that might reveal API versions
rg -B 2 -A 2 'register_rest_route\(' --type php

Length of output: 93


Script:

#!/bin/bash
# Search for namespace property definitions in PHP files
rg -B 2 -A 2 '\$namespace\s*=\s*[\'"][^\'"]*[\'"]' --type php

# Also search for any route registrations that might reveal API versions
rg -B 2 -A 2 'register_rest_route' --type php

Length of output: 93


Script:

#!/bin/bash
# Search for namespace definitions
rg '\$namespace' --type php

# Search for route registrations
rg 'register_rest_route' --type php

Length of output: 9328

includes/REST/TaxClassController.php (2)

1-9: LGTM! Proper namespace, imports and security check.

The file follows best practices with proper namespace declaration, necessary imports, and includes the ABSPATH security check to prevent direct file access.


27-38: Consider validating the request parameter.

While the permission check is implemented correctly, consider validating the $request parameter even though it's not currently used. This future-proofs the method in case request parameters are added later.

Let's check if there are any request parameters being used in the parent class:

includes/REST/ShippingMethodController.php (2)

20-25: LGTM! Proper REST API versioning.

The namespace follows REST API versioning best practices with a clear vendor prefix and version number.


1-61: Verify REST API endpoint registration.

Let's ensure this controller is properly registered in the REST API manager.

✅ Verification successful

Let me gather more information about the REST API registration by checking the class map in the Manager.php file.


Let me fix the regex and try again to check the REST API class map.


Let me check if the ShippingMethodController is registered elsewhere in the Manager.php file since it's not in the initial class map.


Let me verify how these registered controllers are initialized in the Manager class.


Based on the gathered information, I can now provide the final response:

REST API endpoint registration is properly implemented

The ShippingMethodController is correctly registered in the REST API system:

  • It's included in the REST API class map in Manager.php
  • The Manager class properly initializes all registered controllers through the register_rest_routes method
  • The registration happens during WordPress's rest_api_init hook
  • The controller extends WC_REST_Shipping_Methods_Controller and sets its namespace to 'dokan/v1'
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for ShippingMethodController registration
rg -l "ShippingMethodController" | xargs rg "get_rest_api_class_map|rest_api_init"

Length of output: 332


Script:

#!/bin/bash
# Check the REST API class map content
rg -A 20 "get_rest_api_class_map\(\) {" includes/REST/Manager.php

Length of output: 187


Script:

#!/bin/bash
# Check the REST API class map content with fixed regex
rg -A 20 "private function get_rest_api_class_map" includes/REST/Manager.php

Length of output: 2198


Script:

#!/bin/bash
# Check for ShippingMethodController registration in Manager.php
rg -B 5 -A 5 "ShippingMethodController" includes/REST/Manager.php

Length of output: 1462


Script:

#!/bin/bash
# Check the registration mechanism in Manager.php
rg -B 5 -A 10 "register_rest_routes" includes/REST/Manager.php

Length of output: 1327

includes/REST/PaymentGatewayController.php (2)

1-9: LGTM! File structure follows best practices.

The file includes proper namespace declaration, necessary imports, and security check.


20-25: Verify API version number.

The PR objectives mention this is a new Payment Gateway API (v1), so the namespace version is correct. However, let's verify there are no other payment gateway endpoints in different versions.

✅ Verification successful

Based on the search results, I can now generate the final response since I have sufficient information about the API versioning across the codebase.

API version (v1) is consistent with related endpoints

The payment gateway endpoint version is correctly set to v1, which aligns with other related endpoints in the codebase:

  • Other payment/financial related endpoints like WithdrawController and ReverseWithdrawalController use v1
  • Store settings and tax-related endpoints also use v1
  • This is the first payment gateway controller implementation, so starting with v1 is appropriate
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any existing payment gateway endpoint definitions
rg -i "payment.*gateway.*controller" --type php
# Search for dokan API endpoint registrations
rg "namespace.*dokan/v" --type php

Length of output: 3656

tests/php/src/REST/OrderActionsControllerV3Test.php (1)

37-54: 🛠️ Refactor suggestion

Verify actual effects of order actions.

The test only verifies the API response but doesn't confirm that the action was actually performed. Consider:

  1. Adding assertions to verify the email was sent for 'send_order_details'
  2. Checking order meta or status changes
  3. Using WordPress testing hooks to verify action triggers
 public function test_apply_order_action() {
     wp_set_current_user( $this->seller_id1 );
     $order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create();
 
+    // Hook into email sending action to verify it's triggered
+    $email_sent = false;
+    add_action('woocommerce_email_send_order_details', function() use (&$email_sent) {
+        $email_sent = true;
+    });
+
     $response = $this->post_request(
         "orders/{$order_id}/actions",
         ['action' => 'send_order_details']
     );
 
     $this->assertEquals( 200, $response->get_status() );
     $data = $response->get_data();
 
     $this->assertArrayHasKey( 'message', $data );
     $this->assertEquals( 'Order action applied successfully.', $data['message'] );
     $this->assertEquals( 'send_order_details', $data['action'] );
+    $this->assertTrue($email_sent, 'Email should have been triggered');
 }
tests/php/src/REST/OrderRefundsControllerV3Test.php (1)

1-14: LGTM! Well-structured test class setup.

The class follows PHPUnit best practices with proper namespacing, inheritance, and test grouping annotations.

tests/php/src/REST/OrderNotesControllerTestV3.php (1)

1-11: LGTM! Well-structured test file setup.

The file follows proper PHP practices with clear namespace declaration and appropriate test grouping.

includes/REST/OrderRefundControllerV3.php (1)

112-114: Vendor Permission Check Implementation is Correct

The check_vendor_permission method correctly determines if the current user is a vendor, ensuring that only vendor users can access these endpoints.

includes/REST/OrderActionControllerV3.php (1)

221-224: Check for Existence of Email Classes Before Invocation

In the process_order_action method, when resending emails, the code assumes that the email classes WC_Email_Customer_Invoice and WC_Email_New_Order are available. If another plugin unhooks or removes these email classes, it could lead to errors. Consider verifying the existence of these classes before calling their methods.

Run the following script to confirm that the email classes are available:

Also applies to: 252-257

includes/REST/OrderControllerV3.php (2)

38-49: Permission checks are correctly implemented

The check_permission method effectively verifies vendor permissions and returns appropriate error messages for different actions. This ensures secure access control.


85-103: Properly associating orders with the current vendor

When creating a new order, the code correctly sets the _dokan_vendor_id meta to associate the order with the current vendor. This is essential for accurate order management.

Comment on lines +19 to +25
public function test_ensure_route_is_registered() {
$routes = $this->server->get_routes( $this->namespace );
$full_route = $this->get_route( 'taxes/classes' );

$this->assertArrayHasKey( $full_route, $routes );
$this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] );
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance route registration test coverage.

The current test only verifies GET method support. Based on REST API best practices, consider adding tests for:

  1. Other HTTP methods (POST, PUT, DELETE) if supported
  2. Route parameters and their constraints
  3. Authentication requirements

Here's a suggested enhancement:

public function test_ensure_route_is_registered() {
    $routes = $this->server->get_routes( $this->namespace );
    $full_route = $this->get_route( 'taxes/classes' );

    $this->assertArrayHasKey( $full_route, $routes );
    
    // Test all supported methods
    $supported_methods = ['GET', 'POST', 'PUT', 'DELETE'];
    foreach ($supported_methods as $method) {
        $this->assertNestedContains( 
            [ 'methods' => [ $method => true ] ], 
            $routes[ $full_route ] 
        );
    }
    
    // Test permission callback existence
    $this->assertArrayHasKey( 
        'permission_callback',
        $routes[ $full_route ][0]
    );
}

Comment on lines +30 to +38
public function test_get_tax_classes() {
wp_set_current_user( $this->seller_id1 );

$response = $this->get_request( 'taxes/classes' );

$this->assertEquals( 200, $response->get_status() );
$data = $response->get_data();
$this->assertIsArray( $data );
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Insufficient test coverage for tax classes retrieval.

The current test only verifies successful retrieval with 200 status. Consider adding tests for:

  1. Error scenarios (unauthorized access, invalid requests)
  2. Response data structure validation
  3. Different user roles (admin, customer, guest)
  4. Empty tax classes scenario

Here's a suggested enhancement:

public function test_get_tax_classes() {
    // Test unauthorized access
    wp_set_current_user( 0 );
    $response = $this->get_request( 'taxes/classes' );
    $this->assertEquals( 401, $response->get_status() );

    // Test with seller
    wp_set_current_user( $this->seller_id1 );
    $response = $this->get_request( 'taxes/classes' );
    $this->assertEquals( 200, $response->get_status() );
    $data = $response->get_data();
    
    // Validate response structure
    $this->assertIsArray( $data );
    foreach ( $data as $tax_class ) {
        $this->assertArrayHasKey( 'name', $tax_class );
        $this->assertArrayHasKey( 'slug', $tax_class );
    }
}

public function test_get_tax_classes_empty() {
    // Setup empty tax classes
    delete_option( 'woocommerce_tax_classes' );
    
    wp_set_current_user( $this->seller_id1 );
    $response = $this->get_request( 'taxes/classes' );
    
    $this->assertEquals( 200, $response->get_status() );
    $this->assertEmpty( $response->get_data() );
}

Comment on lines +27 to +39
/**
* Test getting taxes.
*/
public function test_get_taxes() {
wp_set_current_user( $this->seller_id1 );

$response = $this->get_request( 'taxes' );

$this->assertEquals( 200, $response->get_status() );
$data = $response->get_data();
$this->assertIsArray( $data );
// Add more assertions based on expected tax data
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Enhance test coverage for tax retrieval.

The current test implementation is incomplete. Consider adding:

  1. Test cases for different user roles (admin, customer, unauthorized)
  2. Validation of the tax data structure and required fields
  3. Error case testing (invalid requests, unauthorized access)
  4. Pagination and filtering tests if supported

Here's a suggested expansion of the test cases:

/**
 * Test getting taxes with different user roles.
 *
 * @dataProvider user_role_provider
 */
public function test_get_taxes_with_different_roles($role, $expected_status) {
    $user_id = $this->factory->user->create(['role' => $role]);
    wp_set_current_user($user_id);

    $response = $this->get_request('taxes');
    $this->assertEquals($expected_status, $response->get_status());
}

/**
 * Test tax data structure and required fields.
 */
public function test_get_taxes_data_structure() {
    wp_set_current_user($this->seller_id1);
    $response = $this->get_request('taxes');
    $data = $response->get_data();

    $this->assertArrayHasKey('tax_rates', $data);
    foreach ($data['tax_rates'] as $tax_rate) {
        $this->assertArrayHasKeys([
            'id',
            'country',
            'state',
            'rate',
            'name',
            'priority',
            'compound',
            'shipping',
            'order'
        ], $tax_rate);
    }
}

/**
 * Data provider for user role tests.
 */
public function user_role_provider() {
    return [
        ['administrator', 200],
        ['seller', 200],
        ['customer', 403],
        ['subscriber', 403]
    ];
}

Comment on lines +14 to +25
/**
* Test ensuring the route is registered.
*
* @return void
*/
public function test_ensure_route_is_registered() {
$routes = $this->server->get_routes( $this->namespace );
$full_route = $this->get_route( 'taxes' );

$this->assertArrayHasKey( $full_route, $routes );
$this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] );
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Expand route registration test coverage.

The test only verifies GET method support, but according to the PR objectives, the tax management API should support CRUD operations. Consider adding assertions for other HTTP methods (POST, PUT, DELETE) that would be needed for tax rate management.

 public function test_ensure_route_is_registered() {
     $routes = $this->server->get_routes( $this->namespace );
     $full_route = $this->get_route( 'taxes' );

     $this->assertArrayHasKey( $full_route, $routes );
-    $this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] );
+    $this->assertNestedContains( 
+        [ 
+            'methods' => [ 
+                'GET' => true,
+                'POST' => true,
+                'PUT' => true,
+                'DELETE' => true 
+            ] 
+        ], 
+        $routes[ $full_route ] 
+    );
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/**
* Test ensuring the route is registered.
*
* @return void
*/
public function test_ensure_route_is_registered() {
$routes = $this->server->get_routes( $this->namespace );
$full_route = $this->get_route( 'taxes' );
$this->assertArrayHasKey( $full_route, $routes );
$this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] );
}
/**
* Test ensuring the route is registered.
*
* @return void
*/
public function test_ensure_route_is_registered() {
$routes = $this->server->get_routes( $this->namespace );
$full_route = $this->get_route( 'taxes' );
$this->assertArrayHasKey( $full_route, $routes );
$this->assertNestedContains(
[
'methods' => [
'GET' => true,
'POST' => true,
'PUT' => true,
'DELETE' => true
]
],
$routes[ $full_route ]
);
}

Comment on lines +1 to +40
<?php

namespace WeDevs\Dokan\Test\REST;

use WeDevs\Dokan\Test\DokanTestCase;

/**
* @group dokan-taxes
*/
class TaxControllerTest extends DokanTestCase {

protected $namespace = 'dokan/v1';

/**
* Test ensuring the route is registered.
*
* @return void
*/
public function test_ensure_route_is_registered() {
$routes = $this->server->get_routes( $this->namespace );
$full_route = $this->get_route( 'taxes' );

$this->assertArrayHasKey( $full_route, $routes );
$this->assertNestedContains( [ 'methods' => [ 'GET' => true ] ], $routes[ $full_route ] );
}

/**
* Test getting taxes.
*/
public function test_get_taxes() {
wp_set_current_user( $this->seller_id1 );

$response = $this->get_request( 'taxes' );

$this->assertEquals( 200, $response->get_status() );
$data = $response->get_data();
$this->assertIsArray( $data );
// Add more assertions based on expected tax data
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Based on the gathered information, I can now provide a final response:

Test coverage needs expansion beyond basic endpoints

After analyzing the codebase, I found that:

  • Current test coverage only includes basic route registration and GET endpoints for both tax rates and tax classes
  • The controllers implement permission checks that are not fully tested
  • The TaxClassController has delete functionality without corresponding test coverage
  • Integration with orders and shipping components is not evident in the test suite

Required test additions:

  • Permission check tests for both controllers
  • Delete operation tests for tax classes
  • Vendor-specific tax settings validation
  • Integration tests with order calculations

Note: The review comment was correct in identifying gaps in test coverage. The implemented controllers have more functionality than what's currently being tested, and integration aspects are not covered in the test suite.

🔗 Analysis chain

Verify test coverage aligns with PR objectives.

The current test file only covers basic tax rate retrieval. Based on the PR objectives, please ensure test coverage for:

  1. Tax class management operations
  2. Vendor-specific tax settings
  3. Integration with orders and shipping components
  4. Permission checks for vendor authorization

Let me gather more information about the test coverage by examining the contents of these files.

This will help me verify if the test coverage aligns with the implemented functionality and PR objectives. Let me analyze the actual implementations and test coverage before making a final assessment.


Let me check for order and shipping integration tests to complete the verification.

This will help verify the integration aspects mentioned in the review comment before making the final assessment.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for other tax-related test files and endpoints

echo "Checking for tax-related test files..."
fd -e php -p "Tax.*Test\.php$" tests/

echo "Checking for tax-related endpoint implementations..."
rg -l "class.*Tax.*Controller" includes/

Length of output: 440


Script:

#!/bin/bash
# Check contents of tax-related test files and their corresponding controllers

echo "=== TaxClassControllerTest.php ==="
cat tests/php/src/REST/TaxClassControllerTest.php

echo -e "\n=== TaxController.php ==="
cat includes/REST/TaxController.php

echo -e "\n=== TaxClassController.php ==="
cat includes/REST/TaxClassController.php

Length of output: 4694


Script:

#!/bin/bash
# Check for tax-related code in orders and shipping tests

echo "=== Searching for tax handling in order tests ==="
rg -l "tax" tests/php/src/Orders --type php

echo -e "\n=== Searching for tax handling in shipping tests ==="
rg -l "tax" tests/php/src/Shipping --type php

echo -e "\n=== Checking for tax-related methods in order controller ==="
rg "function.*tax" includes/REST/OrderController.php

Length of output: 574

Comment on lines +66 to +83
protected function check_order_authorization( int $order_id ) {
$order = wc_get_order( $order_id );

if ( ! $order ) {
return new WP_Error( 'dokan_rest_invalid_order_id', __( 'Invalid order ID.', 'dokan-lite' ), array( 'status' => 404 ) );
}

if ( $order->get_meta( 'has_sub_order' ) ) {
return new WP_Error( 'dokan_rest_invalid_order', __( 'Sorry, this is a parent order', 'dokan-lite' ), array( 'status' => 404 ) );
}

$vendor_id = dokan_get_seller_id_by_order( $order_id );
if ( $vendor_id !== dokan_get_current_user_id() ) {
return new WP_Error( 'dokan_rest_unauthorized_order', __( 'You do not have permission to access this order', 'dokan-lite' ), array( 'status' => 403 ) );
}

return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize Order Retrieval to Avoid Redundant Calls

In the check_order_authorization method, the order is retrieved using wc_get_order( $order_id ). Later, in both the get_items and create_item methods, the same order is retrieved again using wc_get_order( $order_id ). To improve performance and reduce redundancy, consider returning the WC_Order object upon successful authorization. This way, you can reuse the order object without additional retrievals.

Apply this diff to modify the return value:

 protected function check_order_authorization( int $order_id ) {
     $order = wc_get_order( $order_id );
     // ... existing checks ...
-    return true;
+    return $order;
 }

Then update the get_items method:

 public function get_items( $request ) {
     $order_id = $request['id'];
-    $auth_check = $this->check_order_authorization( $order_id );
-    if ( is_wp_error( $auth_check ) ) {
-        return $auth_check;
-    }
-
-    $order = wc_get_order( $order_id );
+    $order = $this->check_order_authorization( $order_id );
+    if ( is_wp_error( $order ) ) {
+        return $order;
+    }
     $actions = $this->get_available_order_actions_for_order( $order );
     return rest_ensure_response( $actions );
 }

Similarly, update the create_item method to utilize the returned order object.

Committable suggestion was skipped due to low confidence.

Comment on lines +192 to +198
public function get_items( $request ): WP_REST_Response {
return $this->perform_vendor_action(
function () use ( $request ) {
return parent::get_items( $request );
}
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider simplifying repetitive code in CRUD methods

The CRUD methods (get_items, create_item, etc.) all use perform_vendor_action to wrap parent methods. To reduce code duplication, consider abstracting this pattern or using a loop or decorator to handle these methods dynamically.

*
* @return WP_REST_Response
*/
public function prepare_object_for_response( $object, $request ): WP_REST_Response {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid using reserved keyword 'object' as a parameter name

In PHP 7 and above, object is a reserved keyword. Using it as a parameter name can lead to parsing errors. Consider renaming the parameter to $order for clarity and to prevent potential issues.

Apply this diff to fix the issue:

-public function prepare_object_for_response( $object, $request ): WP_REST_Response {
+public function prepare_object_for_response( $order, $request ): WP_REST_Response {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public function prepare_object_for_response( $object, $request ): WP_REST_Response {
public function prepare_object_for_response( $order, $request ): WP_REST_Response {
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection

[warning] 286-286:
It is recommended not to use reserved keyword "object" as function parameter name. Found: $object

Comment on lines +655 to +656
$expected_total = (float) ( $product->get_price() * 2 ) * 0.95; // 10% discount
$this->assertEquals( $expected_total, $data['total'] );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix the discount calculation in the expected total

In the test_orders_create_with_vendor_coupon method, the expected total is calculated using a multiplier of 0.95, which corresponds to a 5% discount. Since the coupon provides a 10% discount, the multiplier should be 0.9.

Apply this diff to correct the calculation:

-            $expected_total = (float) ( $product->get_price() * 2 ) * 0.95; // 10% discount
+            $expected_total = (float) ( $product->get_price() * 2 ) * 0.9; // 10% discount
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
$expected_total = (float) ( $product->get_price() * 2 ) * 0.95; // 10% discount
$this->assertEquals( $expected_total, $data['total'] );
$expected_total = (float) ( $product->get_price() * 2 ) * 0.9; // 10% discount
$this->assertEquals( $expected_total, $data['total'] );

Comment on lines +553 to +575
wp_set_current_user( $this->seller_id1 );

// Update commission settings for dokan.
$commission_options = array(
'commission_type' => 'percentage',
'admin_percentage' => 10,
'additional_fee' => 0,
);
update_option( 'dokan_selling', $commission_options );

$order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create();
$response = $this->get_request( 'orders/' . $order_id );

$this->assertEquals( 200, $response->get_status() );

$order = $response->get_data();

$this->assertArrayHasKey( 'line_items', $order );

foreach ( $order['line_items'] as $line_item ) {
$this->assertArrayHasKey( 'meta_data', $order );
$this->assertGreaterThanOrEqual( 3, count( $order['meta_data'] ) );

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correct the access to 'meta_data' in line items

Within the test_order_commission_by_vendor method, the assertions inside the loop over $order['line_items'] are accessing $order['meta_data'] instead of $line_item['meta_data']. To accurately validate each line item's metadata, the code should access $line_item['meta_data'].

Apply this diff to fix the metadata assertions:

-                $this->assertArrayHasKey( 'meta_data', $order );
-                $this->assertGreaterThanOrEqual( 3, count( $order['meta_data'] ) );
+                $this->assertArrayHasKey( 'meta_data', $line_item );
+                $this->assertGreaterThanOrEqual( 3, count( $line_item['meta_data'] ) );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
wp_set_current_user( $this->seller_id1 );
// Update commission settings for dokan.
$commission_options = array(
'commission_type' => 'percentage',
'admin_percentage' => 10,
'additional_fee' => 0,
);
update_option( 'dokan_selling', $commission_options );
$order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create();
$response = $this->get_request( 'orders/' . $order_id );
$this->assertEquals( 200, $response->get_status() );
$order = $response->get_data();
$this->assertArrayHasKey( 'line_items', $order );
foreach ( $order['line_items'] as $line_item ) {
$this->assertArrayHasKey( 'meta_data', $order );
$this->assertGreaterThanOrEqual( 3, count( $order['meta_data'] ) );
wp_set_current_user( $this->seller_id1 );
// Update commission settings for dokan.
$commission_options = array(
'commission_type' => 'percentage',
'admin_percentage' => 10,
'additional_fee' => 0,
);
update_option( 'dokan_selling', $commission_options );
$order_id = $this->factory()->order->set_seller_id( $this->seller_id1 )->create();
$response = $this->get_request( 'orders/' . $order_id );
$this->assertEquals( 200, $response->get_status() );
$order = $response->get_data();
$this->assertArrayHasKey( 'line_items', $order );
foreach ( $order['line_items'] as $line_item ) {
$this->assertArrayHasKey( 'meta_data', $line_item );
$this->assertGreaterThanOrEqual( 3, count( $line_item['meta_data'] ) );

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 21

🧹 Outside diff range and nitpick comments (8)
includes/REST/OrderNoteControllerV3.php (1)

91-91: Fix grammatical error in docblock

In the docblock for delete_item_permissions_check, there's a grammatical error: "Check if a given request has access delete a order note." It should be "Check if a given request has access to delete an order note."

Apply this diff to correct the error:

 /**
- * Check if a given request has access delete a order note.
+ * Check if a given request has access to delete an order note.
  *
  * @param WP_REST_Request $request Full details about the request.
  * @return bool|WP_Error
  */
tests/php/src/REST/DataCountriesControllerTest.php (2)

168-185: Assert status code in concurrent requests test.

In test_concurrent_requests, consider asserting that each response has a status code of 200 to ensure all requests are successful, in addition to comparing the response data.

Apply this diff to include the assertions:

    foreach ( $responses as $response ) {
+       $this->assertEquals( 200, $response->get_status() );
        $this->assertEquals(
            $first_response->get_data(),
            $response->get_data()
        );
    }

96-105: Verify country code and name in empty states test.

In test_get_states_empty_states, you're verifying that the states array is empty for country FR. Consider also asserting that the code and name in the response match the expected country to ensure the correct country data is returned.

Apply this diff to include the assertions:

    $this->assertArrayHasKey( 'states', $data );
+   $this->assertEquals( 'FR', $data['code'] );
+   $this->assertEquals( 'France', $data['name'] );
    $this->assertEmpty( $data['states'] );
tests/php/src/REST/OrderNoteControllerV3Test.php (2)

95-118: Enhance test assertions to verify note content

While the test checks the structure of the returned notes, it does not verify the actual content. To ensure the correct notes are retrieved, consider adding assertions to check the content of each note.

Example additions:

// Verify notes are returned
$this->assertCount( 2, $data );

// Verify the content of the notes
$this->assertEquals( 'Test note 1', $data[0]['note'] );
$this->assertEquals( 'Test note 2', $data[1]['note'] );

316-344: Rename test method to reflect sequential execution

The method test_concurrent_notes suggests testing concurrent operations, but the notes are created sequentially in a loop. Since PHP executes code sequentially in this context, consider renaming the method to better reflect the test's purpose.

Apply this diff to rename the method:

 /**
- * Test concurrent note operations
+ * Test multiple note creation
  */
-public function test_concurrent_notes() {
+public function test_multiple_note_creation() {
tests/php/src/REST/OrderRefundControllerV3Test.php (3)

194-210: Ensure full refund tests expect correct status codes.

The test_create_full_refund method expects a 500 status code when creating a full refund. Typically, a full refund should either be processed successfully (201 Created) or return a client error like 400 Bad Request if not allowed. Relying on a 500 Internal Server Error may not be appropriate as it indicates a server-side issue.

Consider adjusting the test to expect a more suitable status code or handling full refund scenarios correctly in the API.


169-189: Enhance assertions for invalid refund amount tests.

In the test_create_refund_invalid_amount method, while you are asserting the status codes, adding assertions to check the error codes and messages will ensure that the API provides clear feedback and that the correct error handling paths are tested.


315-315: Verify refund creation before proceeding with deletion.

In the test_delete_refund method, after creating a refund, it's good practice to check that the refund was created successfully before attempting to delete it. This will make the test more robust and easier to debug if refund creation fails.

Consider adding an assertion to verify the refund ID is valid.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 18c57e1 and ee2ad90.

📒 Files selected for processing (8)
  • includes/REST/Manager.php (1 hunks)
  • includes/REST/OrderActionControllerV3.php (1 hunks)
  • includes/REST/OrderNoteControllerV3.php (1 hunks)
  • includes/REST/OrderRefundControllerV3.php (1 hunks)
  • tests/php/src/REST/DataCountriesControllerTest.php (1 hunks)
  • tests/php/src/REST/OrderActionControllerV3Test.php (1 hunks)
  • tests/php/src/REST/OrderNoteControllerV3Test.php (1 hunks)
  • tests/php/src/REST/OrderRefundControllerV3Test.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • includes/REST/Manager.php
🔇 Additional comments (3)
tests/php/src/REST/OrderNoteControllerV3Test.php (1)

1-345: Well-structured test class with comprehensive coverage

The OrderNoteControllerV3Test class is well-designed, providing thorough coverage of various scenarios related to order note management. The tests include authorization checks, input validation, successful operations, and error handling, ensuring robust verification of the controller's functionality.

includes/REST/OrderActionControllerV3.php (1)

313-320: Incorrect Placement of required in JSON Schema

The required keyword should be placed at the root level of the schema, not within the action property definition. This ensures compliance with JSON Schema Draft-04 standards.

tests/php/src/REST/OrderRefundControllerV3Test.php (1)

208-210: ⚠️ Potential issue

Investigate unexpected 500 error when creating a full refund.

In the test_create_full_refund method, asserting a 500 Internal Server Error when attempting to create a full refund might indicate an issue with the refund process. A 500 error suggests an unhandled exception or server error, which is not expected for this operation. Consider reviewing why a full refund results in a 500 error and ensure that the API handles this case gracefully, possibly with a more appropriate status code like 400 Bad Request.

Please run the following script to investigate any discrepancies in handling full refunds:

Verify whether the API returns a 500 error or handles the full refund appropriately.

✅ Verification successful

Let me analyze the test file to better understand the context since the curl command didn't provide useful information. Let me generate some scripts to investigate the test case and related code.


Let me check the actual controller implementation to understand how the refund is being handled.


Based on the code analysis, I can now provide a final response regarding the 500 error assertion in the test case.

The 500 error assertion in the test is correct and expected behavior

The test is asserting a 500 error with the code 'woocommerce_rest_cannot_create_order_refund' which is the correct behavior because:

  1. The OrderRefundControllerV3 extends WooCommerce's WC_REST_Order_Refunds_Controller and performs additional vendor permission checks before allowing refund operations.

  2. The controller implements strict authorization checks through:

    • check_vendor_permission()
    • check_order_authorization()
    • Various permission check methods for different operations
  3. When a refund creation fails due to authorization or validation issues, WooCommerce's parent controller returns a 500 error with the specific error code, which is the expected behavior being tested.

The test is correctly verifying that unauthorized or invalid refund attempts are properly rejected with the appropriate error response.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if full refunds are handled properly without resulting in a server error.

# Test: Attempt to create a full refund via the REST API and check the response.
# Expected Result: The API should allow full refunds or return a clear error message without a 500 status code.

curl -X POST "https://your-site.com/wp-json/dokan/v3/orders/{$this->order_id}/refunds" \
     -H "Content-Type: application/json" \
     -d '{"amount": "'$total'", "reason": "Testing full refund handling"}' \
     -u seller_username:seller_password \
     -i

Length of output: 7712


Script:

#!/bin/bash
# First, let's look at the full test method and surrounding context
rg -B 5 -A 10 "test_create_full_refund" tests/php/src/REST/OrderRefundControllerV3Test.php

# Then let's check the actual controller implementation
fd -e php OrderRefundControllerV3.php

# Also check for any related refund handling code
rg -l "create.*refund|process.*refund" --type php

Length of output: 843


Script:

#!/bin/bash
# Check the controller implementation for refund handling
cat includes/REST/OrderRefundControllerV3.php

# Also check the Order Hooks implementation since it appeared in the refund-related files
cat includes/Order/Hooks.php

Length of output: 28321

Comment on lines +54 to +60
public function get_items_permissions_check( $request ) {
if ( ! $this->check_vendor_permission() ) {
return new WP_Error( 'dokan_rest_cannot_view', __( 'Sorry, you cannot list resources.', 'dokan-lite' ), array( 'status' => rest_authorization_required_code() ) );
}

return $this->check_order_authorization( $request['order_id'] );
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add type hints for $request parameters

Consider adding type hints for the $request parameter in your methods to enhance code clarity and enable static analysis tools to catch type-related issues.

Apply this diff to add the WP_REST_Request type hint:

-public function get_items_permissions_check( $request ) {
+public function get_items_permissions_check( WP_REST_Request $request ) {

-public function create_item_permissions_check( $request ) {
+public function create_item_permissions_check( WP_REST_Request $request ) {

-public function get_item_permissions_check( $request ) {
+public function get_item_permissions_check( WP_REST_Request $request ) {

-public function delete_item_permissions_check( $request ) {
+public function delete_item_permissions_check( WP_REST_Request $request ) {

-public function create_item( $request ) {
+public function create_item( WP_REST_Request $request ) {

Also applies to: 68-74, 82-88, 96-102, 119-119

Comment on lines +41 to +45
$vendor_id = dokan_get_seller_id_by_order( $order_id );
if ( $vendor_id !== dokan_get_current_user_id() ) {
return new WP_Error( 'dokan_rest_unauthorized_order', __( 'You do not have permission to access this order', 'dokan-lite' ), array( 'status' => 403 ) );
}
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure $vendor_id is valid before comparison

In check_order_authorization, ensure that $vendor_id is valid before comparing it with the current user ID to prevent potential issues when $vendor_id is null or false.

Apply this diff to add a validity check for $vendor_id:

 $vendor_id = dokan_get_seller_id_by_order( $order_id );
+if ( empty( $vendor_id ) ) {
+    return new WP_Error( 'dokan_rest_invalid_vendor', __( 'Invalid vendor ID.', 'dokan-lite' ), array( 'status' => 403 ) );
+}
 if ( $vendor_id !== dokan_get_current_user_id() ) {
     return new WP_Error( 'dokan_rest_unauthorized_order', __( 'You do not have permission to access this order', 'dokan-lite' ), array( 'status' => 403 ) );
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
$vendor_id = dokan_get_seller_id_by_order( $order_id );
if ( $vendor_id !== dokan_get_current_user_id() ) {
return new WP_Error( 'dokan_rest_unauthorized_order', __( 'You do not have permission to access this order', 'dokan-lite' ), array( 'status' => 403 ) );
}
return true;
$vendor_id = dokan_get_seller_id_by_order( $order_id );
if ( empty( $vendor_id ) ) {
return new WP_Error( 'dokan_rest_invalid_vendor', __( 'Invalid vendor ID.', 'dokan-lite' ), array( 'status' => 403 ) );
}
if ( $vendor_id !== dokan_get_current_user_id() ) {
return new WP_Error( 'dokan_rest_unauthorized_order', __( 'You do not have permission to access this order', 'dokan-lite' ), array( 'status' => 403 ) );
}
return true;

Comment on lines +59 to +80
public function test_get_items_success() {
wp_set_current_user( $this->seller_id1 );
$response = $this->get_request( $this->base_route );

$this->assertEquals( 200, $response->get_status() );
$data = $response->get_data();

// Verify response structure
$this->assertIsArray( $data );
$this->assertNotEmpty( $data );

// Check first country format
$first_country = reset( $data );
$this->assertArrayHasKey( 'code', $first_country );
$this->assertArrayHasKey( 'name', $first_country );

// Verify some known countries exist
$country_codes = array_column( $data, 'code' );
$this->assertContains( 'US', $country_codes );
$this->assertContains( 'GB', $country_codes );
$this->assertContains( 'CA', $country_codes );
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding tests for pagination and filtering in countries endpoint.

In test_get_items_success, you're testing the retrieval of all countries. If the endpoint supports pagination or filtering, consider adding tests to verify these functionalities to ensure comprehensive coverage.

Comment on lines +158 to +163
public function test_get_states_special_characters() {
wp_set_current_user( $this->seller_id1 );

$response = $this->get_request( $this->base_route . '/US@#' );
$this->assertEquals( 404, $response->get_status() );
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add assertion for error code in special characters test.

In test_get_states_special_characters, you assert that the status code is 404. Consider adding an assertion to verify the error code returned in the response to ensure consistent error handling.

Apply this diff to include the assertion:

    $this->assertEquals( 404, $response->get_status() );
+   $this->assertEquals( 'woocommerce_rest_data_invalid_location', $response->get_data()['code'] );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public function test_get_states_special_characters() {
wp_set_current_user( $this->seller_id1 );
$response = $this->get_request( $this->base_route . '/US@#' );
$this->assertEquals( 404, $response->get_status() );
}
public function test_get_states_special_characters() {
wp_set_current_user( $this->seller_id1 );
$response = $this->get_request( $this->base_route . '/US@#' );
$this->assertEquals( 404, $response->get_status() );
$this->assertEquals( 'woocommerce_rest_data_invalid_location', $response->get_data()['code'] );
}

Comment on lines +128 to +133
private function perform_vendor_action( callable $action ) {
add_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
$result = $action();
remove_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
return $result;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure filter is always removed in perform_vendor_action

In the perform_vendor_action method, if an exception or error occurs during the execution of the $action callable, the remove_filter function may not be called, leaving the filter attached and potentially affecting other operations. To ensure that the filter is always removed, wrap the action execution within a try-finally block.

Apply this diff to modify the method:

 private function perform_vendor_action( callable $action ) {
     add_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
-    $result = $action();
-    remove_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
+    try {
+        $result = $action();
+    } finally {
+        remove_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
+    }
     return $result;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private function perform_vendor_action( callable $action ) {
add_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
$result = $action();
remove_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
return $result;
}
private function perform_vendor_action( callable $action ) {
add_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
try {
$result = $action();
} finally {
remove_filter( 'woocommerce_rest_check_permissions', [ $this, 'check_vendor_permission' ] );
}
return $result;
}

Comment on lines +73 to +75
if ( $order->get_meta( 'has_sub_order' ) ) {
return new WP_Error( 'dokan_rest_invalid_order', __( 'Sorry, this is a parent order', 'dokan-lite' ), array( 'status' => 404 ) );
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use Appropriate HTTP Status Code

Returning a 404 Not Found status code for a parent order may not be appropriate since the order exists but cannot be accessed in this context. Consider using a 403 Forbidden status code to indicate that the user is not permitted to access parent orders.

$this->create_refund( $this->order_id, 25 );

$response = $this->get_request( "orders/{$this->order_id}/refunds" );
var_dump( $response->get_data() );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove var_dump debugging statement.

The var_dump statement at line 135 appears to be a leftover from debugging and should be removed to keep the test code clean and output free from unnecessary information.

Apply this diff to remove the var_dump statement:

-            var_dump( $response->get_data() );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var_dump( $response->get_data() );

]
);

var_dump( $response->get_data() );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove var_dump debugging statement.

The var_dump statement at line 237 is unnecessary in the committed test code and should be removed to maintain code cleanliness.

Apply this diff to remove the var_dump statement:

-            var_dump( $response->get_data() );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var_dump( $response->get_data() );

[ 'force' => true ]
);

var_dump( $response->get_data() );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove var_dump debugging statement.

The var_dump statement at line 322 should be removed to prevent unwanted output during test execution.

Apply this diff to remove the var_dump statement:

-            var_dump( $response->get_data() );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var_dump( $response->get_data() );

Comment on lines +400 to +410
private function create_refund( $order_id, $amount ) {
$refund = wc_create_refund(
[
'amount' => $amount,
'reason' => 'Test refund',
'order_id' => $order_id,
]
);

return $refund->get_id();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling in create_refund helper method.

The create_refund method does not handle cases where the refund creation fails (e.g., returns false or a WP_Error). Adding error handling will make tests more robust and provide clearer failure messages.

Apply this diff to include error handling:

 private function create_refund( $order_id, $amount ) {
     $refund = wc_create_refund(
         [
             'amount'   => $amount,
             'reason'   => 'Test refund',
             'order_id' => $order_id,
         ]
     );

+    if ( is_wp_error( $refund ) ) {
+        $this->fail( 'Failed to create refund: ' . $refund->get_error_message() );
+    } elseif ( ! $refund ) {
+        $this->fail( 'Failed to create refund: Unknown error.' );
+    }

     return $refund->get_id();
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private function create_refund( $order_id, $amount ) {
$refund = wc_create_refund(
[
'amount' => $amount,
'reason' => 'Test refund',
'order_id' => $order_id,
]
);
return $refund->get_id();
}
private function create_refund( $order_id, $amount ) {
$refund = wc_create_refund(
[
'amount' => $amount,
'reason' => 'Test refund',
'order_id' => $order_id,
]
);
if ( is_wp_error( $refund ) ) {
$this->fail( 'Failed to create refund: ' . $refund->get_error_message() );
} elseif ( ! $refund ) {
$this->fail( 'Failed to create refund: Unknown error.' );
}
return $refund->get_id();
}

@mralaminahamed mralaminahamed changed the base branch from develop to update/vendor-dashboard-structure October 25, 2024 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Dev Review It requires a developer review and approval Needs: Testing This requires further testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants